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

Add ability to directly configure the S3 ACL #420

Merged
merged 5 commits into from
Aug 5, 2021

Conversation

atyndall
Copy link
Contributor

@atyndall atyndall commented Aug 4, 2021

Currently, the only mechanism to control the permissions of objects in S3 is via the config.fog_public option, which is somewhat limited.

This PR creates the AssetSync.config.aws_acl option, which allows the S3 Access Control List for uploaded objects to be set to specific values.

Depending on the situation, setting these more specific values can be quite useful. For instance, config.aws_acl = 'bucket-owner-full-control' is used in some situations to ensure that the file uploaded is accessible by the bucket's owner, which may differ from the uploader.

We need this option as the account we are uploading from is different to that which owns the bucket, so we must ensure these ACL permissions are correctly set.

Setting this config value will override the AssetSync.config.fog_public setting if set, as it is more specific.

This allows `AssetSync.config.aws_acl` to be set to values like 'bucket-owner-full-control', which can be important for cross-account bucket syncing.
Setting this config value will override the `AssetSync.config.fog_public` setting if set, as it is more specific.
Copy link
Member

@PikachuEXE PikachuEXE left a comment

Choose a reason for hiding this comment

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

Please also see what changes are made for aws_signature_version
And make the same changes for aws_acl

And please also update the README about this new attribute
(I do notice aws_signature_version and maybe other attributes are also not mentioned in README)
Setting this new attribute overrides public option so I think it's worth being mentioned more explicitly

lib/asset_sync/config.rb Outdated Show resolved Hide resolved
@atyndall
Copy link
Contributor Author

atyndall commented Aug 5, 2021

@PikachuEXE I have made the changes requested. Please let me know if there is anything else.

Copy link
Member

@PikachuEXE PikachuEXE left a comment

Choose a reason for hiding this comment

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

LGTM
I will add missing stuff if I spot any after merge
Probably will be released this week/today

@PikachuEXE PikachuEXE merged commit ab116e3 into AssetSync:master Aug 5, 2021
@PikachuEXE
Copy link
Member

Just released in 2.15.0

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.

2 participants