Allow admin to configure bucket ACL settings #600

Open
darkkirb wants to merge 1 commit from darkkirb/akkoma:add-s3-acl-option into develop
Contributor

To upload to Backblaze B2, the ACL needs to match bucket settings, and
if you have a CDN in front of your media server you may want to restrict
public access to the bucket to avoid expensive non-cdn transfers and
bucket enumeration.

To upload to Backblaze B2, the ACL needs to match bucket settings, and if you have a CDN in front of your media server you may want to restrict public access to the bucket to avoid expensive non-cdn transfers and bucket enumeration.
darkkirb force-pushed add-s3-acl-option from 90db8cca7c to e159a03b43 2023-07-23 07:27:32 +00:00 Compare
floatingghost requested changes 2023-07-24 10:17:13 +00:00
floatingghost left a comment
Owner

this looks ok apart from the obvious, but any chance you could add a test for it? over at https://akkoma.dev/AkkomaGang/akkoma/src/branch/develop/test/pleroma/uploaders/s3_test.exs#L56 - assert_called will be your best friend here

ideally before raising a PR you'd run the tests (mix test), which would have flagged up your typo 🥴

this looks ok apart from the obvious, but any chance you could add a test for it? over at https://akkoma.dev/AkkomaGang/akkoma/src/branch/develop/test/pleroma/uploaders/s3_test.exs#L56 - assert_called will be your best friend here ideally before raising a PR you'd run the tests (mix test), which would have flagged up your typo 🥴
@ -576,6 +576,7 @@ the source code is here: [kocaptcha](https://github.com/koto-bank/kocaptcha). Th
Don't forget to configure [Ex AWS S3](#ex-aws-s3-settings)
* `acl`: The ACL the uploaded media will have. By default media will be `:public`, and thus directly accessible from the bucket. Set this to `:private` if you have a CDN in front of your media server which sends authenticated requests to your S3 provider. On some providers, like Backblaze B2, this needs to match bucket settings!

can you also document the old standard value of public_read?

used if you point media directly at the bucket

can you also document the old standard value of `public_read`? used if you point media directly at the bucket
@ -25,6 +25,7 @@ defmodule Pleroma.Uploaders.S3 do
config = Config.get([__MODULE__])
bucket = Keyword.get(config, :bucket)
streaming = Keyword.get(config, :streaming_enabled)
acl = Keyboard.get(config, :acl)

keyboard
keyboard

muscle memory fun

keyboard keyboard muscle memory fun
darkkirb force-pushed add-s3-acl-option from e159a03b43 to 9571f9729d 2023-07-25 05:51:18 +00:00 Compare
darkkirb force-pushed add-s3-acl-option from 9571f9729d to d877856527 2023-07-25 06:01:01 +00:00 Compare
Author
Contributor

oh oops noticed that :public was in fact the wrong value. Added tests for the default value and the new value now, and corrected the two mistakes

oh oops noticed that :public was in fact the wrong value. Added tests for the default value and the new value now, and corrected the two mistakes
darkkirb requested review from floatingghost 2023-07-25 06:06:16 +00:00
floatingghost requested changes 2023-07-27 12:45:32 +00:00
floatingghost left a comment
Owner

once again this does not pass tests

PLEASE make sure you run tests locally before requesting review

once again this does not pass tests _PLEASE_ make sure you run tests locally before requesting review
@ -68,2 +68,3 @@
test "save file", %{file_upload: file_upload} do
with_mock ExAws, request: fn _ -> {:ok, :ok} end do
with_mock ExAws, request: (fn _ ->
assert op.opts[:acl] == :public_read

op is not defined

op is not defined
Some checks failed
ci/woodpecker/pr/woodpecker Pipeline failed
This pull request can be merged automatically.
This branch is out-of-date with the base branch
You are not authorized to merge this pull request.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u add-s3-acl-option:darkkirb-add-s3-acl-option
git checkout darkkirb-add-s3-acl-option
Sign in to join this conversation.
No description provided.