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

Use Django storage backend to handle file-upload to s3 storage #390

Merged
merged 15 commits into from
Jan 17, 2024
Merged

Use Django storage backend to handle file-upload to s3 storage #390

merged 15 commits into from
Jan 17, 2024

Conversation

fugal-dy
Copy link
Contributor

@fugal-dy fugal-dy commented Nov 21, 2023 •
edited by Yelinz
Loading

  • add django-storages with s3 (boto3-backed)

  • change file model:

    • new FileField: content
    • get rid of attributes related to the storage-client method (upload_url, download_url etc)
    • migrate existing data (basically set the file name in a data migration)

  • setup s3 backend configuration for file upload and encryption

    • dynamic storage selection based on File.encryption_status

  • add view-action for file-fetching via backend

  • 100% coverage

  • fix all tests

  • provide a presigned download url for unauthenticated file retrieval

  • manage command for fetching unencrypted files and reupload encrypted

  • per object encryption based on object name and secret

@fugal-dy fugal-dy requested review from czosel and winged November 21, 2023 11:44
@fugal-dy fugal-dy changed the title Use Django storage backend to handle file-upload to s3 storage Draft: Use Django storage backend to handle file-upload to s3 storage Nov 21, 2023
czosel
czosel reviewed Nov 21, 2023
View reviewed changes
czosel
czosel reviewed Nov 21, 2023
View reviewed changes
alexandria/core/serializers.py
if (original := self.initial_data.get("original")) and not isinstance(
original, dict
):
self.initial_data["original"] = {"type": "files", "id": original}
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this get any easier with the help of FormDataResourceRelatedField? See https://github.com/inosca/ebau/blob/main/django/camac/relations.py#L5

@fugal-dy fugal-dy force-pushed the feat-upload-file-to-s3storage branch 2 times, most recently from e1a76ba to 864630b Compare November 24, 2023 13:04
winged
winged requested changes Nov 27, 2023
View reviewed changes
Copy link
Contributor

@winged winged left a comment

Choose a reason for hiding this comment

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

Found a few issues in the code, but mainly smaller ones. The big issues however are:

  • Did you verify the data uploaded in the old way is still accessible after the update? That needs to work absolutely certainly, otherwise we'll need a migration of sorts.
  • I would like to see the virtualenv/poetry stuff split out into it's own merge request, so we can discuss it separately. I'm not 100% convinced it's the right way, but willing to discuss
  • And last: The change needs to be marked breaking, as it's entirely different config, and entirely different way of accessing data for the frontend

@fugal-dy fugal-dy force-pushed the feat-upload-file-to-s3storage branch 7 times, most recently from 16af39a to 4ac5401 Compare November 29, 2023 11:28
@fugal-dy fugal-dy requested a review from winged November 29, 2023 11:53
@fugal-dy fugal-dy force-pushed the feat-upload-file-to-s3storage branch 5 times, most recently from 41291e8 to 2d87a24 Compare November 29, 2023 17:35
winged
winged requested changes Nov 30, 2023
View reviewed changes
Copy link
Contributor

@winged winged left a comment

Choose a reason for hiding this comment

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

Nice work so far, getting there!

I'd like to see the whole encryption thing to be optional and controllable via settings. Right now you can't really have unencrypted files in S3

@fugal-dy fugal-dy force-pushed the feat-upload-file-to-s3storage branch 3 times, most recently from 5bb47e5 to b8c09fe Compare November 30, 2023 21:59
winged
winged requested changes Dec 1, 2023
View reviewed changes
@czosel czosel mentioned this pull request Dec 8, 2023
Merged
@fugal-dy fugal-dy force-pushed the feat-upload-file-to-s3storage branch 3 times, most recently from 99068a1 to d2f714d Compare December 13, 2023 16:38
@fugal-dy fugal-dy requested a review from winged December 14, 2023 14:01
Yelinz
Yelinz reviewed Jan 8, 2024
View reviewed changes
@anehx anehx mentioned this pull request Jan 9, 2024
Merged
@Yelinz Yelinz force-pushed the feat-upload-file-to-s3storage branch 2 times, most recently from bdc3b06 to f91fd43 Compare January 10, 2024 13:32
@Yelinz Yelinz force-pushed the feat-upload-file-to-s3storage branch from f91fd43 to e380f1a Compare January 11, 2024 15:54
@Yelinz Yelinz force-pushed the feat-upload-file-to-s3storage branch from e380f1a to a26a939 Compare January 15, 2024 07:40
winged
winged requested changes Jan 15, 2024
View reviewed changes
Copy link
Contributor

@winged winged left a comment

Choose a reason for hiding this comment

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

Nice work! I think after some final cleanup, this is on a good track 👍

README.md Outdated
- `FILE_STORAGE_BACKEND`: Set the backend for file uploads. `django-storages` is available (default: `django.core.files.storage.FileSystemStorage`)

Encryption:
- `ALEXANDRIA_ENABLE_AT_REST_ENCRYPTION`: Set to `true` to enable at-rest encryption of files (does nothing by itself unless `ALEXANDRIA_ENCRYPTRION_METHOD` is set to a supported method)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is dangerous. Enabling encryption should force you to configure everything needed. You shouldn't be able to set ALEXANDRIA_ENABLE_AT_REST_ENCRYPTION=True and allow unencrypted files to be stored

Copy link
Member

Choose a reason for hiding this comment

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

Added a check

alexandria/alexandria/storages/fields.py

Lines 39 to 42 in e941029

elif method == File.EncryptionStatus.NOT_ENCRYPTED:
raise ImproperlyConfigured(
"ALEXANDRIA_ENCRYPTION_METHOD is set to NOT_ENCRYPTED while ALEXANDRIA_ENABLE_AT_REST_ENCRYPTION is enabled."
)

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! But now the docs are not accurate anymore :-)

Suggested change
- `ALEXANDRIA_ENABLE_AT_REST_ENCRYPTION`: Set to `true` to enable at-rest encryption of files (does nothing by itself unless `ALEXANDRIA_ENCRYPTRION_METHOD` is set to a supported method)
- `ALEXANDRIA_ENABLE_AT_REST_ENCRYPTION`: Set to `true` to enable at-rest encryption of files (enabling this causes an error unless `ALEXANDRIA_ENCRYPTRION_METHOD` is set to a supported method)

@Yelinz Yelinz force-pushed the feat-upload-file-to-s3storage branch 3 times, most recently from de1faa2 to 2875125 Compare January 16, 2024 09:39
winged
winged requested changes Jan 16, 2024
View reviewed changes
Copy link
Contributor

@winged winged left a comment

Choose a reason for hiding this comment

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

Any minute now :-)

README.md Outdated
- `FILE_STORAGE_BACKEND`: Set the backend for file uploads. `django-storages` is available (default: `django.core.files.storage.FileSystemStorage`)

Encryption:
- `ALEXANDRIA_ENABLE_AT_REST_ENCRYPTION`: Set to `true` to enable at-rest encryption of files (does nothing by itself unless `ALEXANDRIA_ENCRYPTRION_METHOD` is set to a supported method)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! But now the docs are not accurate anymore :-)

Suggested change
- `ALEXANDRIA_ENABLE_AT_REST_ENCRYPTION`: Set to `true` to enable at-rest encryption of files (does nothing by itself unless `ALEXANDRIA_ENCRYPTRION_METHOD` is set to a supported method)
- `ALEXANDRIA_ENABLE_AT_REST_ENCRYPTION`: Set to `true` to enable at-rest encryption of files (enabling this causes an error unless `ALEXANDRIA_ENCRYPTRION_METHOD` is set to a supported method)

@winged
Copy link
Contributor

winged commented Jan 17, 2024

LGTM now. Are you gonna cleanup the commits a bit before merging? Seems like quite a long chain of fixes...

@Yelinz Yelinz force-pushed the feat-upload-file-to-s3storage branch 2 times, most recently from 847c793 to 6ec30c2 Compare January 17, 2024 09:52
dependabot bot and others added 4 commits January 17, 2024 10:56
@dependabot @Yelinz
chore(deps-dev): bump gitpython from 3.1.40 to 3.1.41 (#414)
2ae692b 
Bumps [gitpython](https://github.com/gitpython-developers/GitPython) from 3.1.40 to 3.1.41.
- [Release notes](https://github.com/gitpython-developers/GitPython/releases)
- [Changelog](https://github.com/gitpython-developers/GitPython/blob/main/CHANGES)
- [Commits](gitpython-developers/GitPython@3.1.40...3.1.41)

---
updated-dependencies:
- dependency-name: gitpython
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@Yelinz
feat(document): update modfied data when creating new file (#416)
7887c62
@Yelinz
chore: v3.0.0-beta.2 (#417)
c8e67b7
@Yelinz
fix: various small fixes for ssec
43c220e 
enable cleanup for storage attribute
add checks to prevent misconfigured usage
complete coverage
configure containers to allow ssec in dev env
add encryption management command test
finalize encryption management command
@Yelinz Yelinz force-pushed the feat-upload-file-to-s3storage branch from 6ec30c2 to 43c220e Compare January 17, 2024 10:00
@Yelinz Yelinz force-pushed the feat-upload-file-to-s3storage branch from 4e313ca to 4f39f90 Compare January 17, 2024 10:05
@Yelinz
Copy link
Member

Yelinz commented Jan 17, 2024

trying to merge the commits messed with the history, because there are merges in between.
Rebasing to remove those again is very tedious...

@Yelinz Yelinz merged commit 13b51f1 into main Jan 17, 2024
3 checks passed
@Yelinz Yelinz mentioned this pull request Jan 17, 2024
Open
@open-dynaMIX open-dynaMIX deleted the feat-upload-file-to-s3storage branch January 18, 2024 07:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Yelinz Yelinz Yelinz left review comments

@winged winged winged approved these changes

@czosel czosel Awaiting requested review from czosel

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@fugal-dy @winged @Yelinz @czosel