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

Better object ACL support #57

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

mgriego
Copy link

@mgriego mgriego commented May 3, 2017

Allow the default bucket ACL to be applied when visibility is not explicitly specified, and copy the actual ACL when copying an object.

…licitly specified, and copy the actual ACL when copying an object.
@mgriego
Copy link
Author

mgriego commented May 3, 2017

Still need to update the tests...

@mgriego
Copy link
Author

mgriego commented May 3, 2017

@matthewgoslett All tests here pass except for the PHP nightly. That failure is actually due to changes in the count() function and the fact that the older version of Mockery is performing a count(null) when we use the withNoArgs() check.

We have a couple of options here:

  1. don't use the withNoArgs() check.
  2. Move the minimum supported version of the project to 5.6 (5.5 went EOL almost a year ago) and update the versions phpunit and Mockery used in the project.
  3. Ignore the nightly failure.

@mgriego
Copy link
Author

mgriego commented May 3, 2017

Also, @matthewgoslett, I made the default when visibility is not explicit to not set an ACL (predefined or otherwise) so that the bucket's default ACL would be applied as expected. I realize that this could potentially be a breaking change and am open to making this behavior dependent on an extra boolean parameter to the constructor (that defaults to the existing behavior) if you think that's prudent, though I would argue that this behavior is more correct.

Reference: https://cloud.google.com/storage/docs/json_api/v1/defaultObjectAccessControls

@matthewgoslett
Copy link
Contributor

Is there any expectation from flysystem as to what the default visibility should be?

@mgriego
Copy link
Author

mgriego commented May 22, 2017

@matthewgoslett Not in the base adapters. If you look at the local and FTP adapters, for instance, the visibility is only set if visibility is defined in the write, etc calls or via a default in the filesystem config. Otherwise, Flysystem won't force a given permission, letting the default filesystem umask/etc take effect.

@mgriego
Copy link
Author

mgriego commented May 22, 2017

Same is actually true for the S3 adapter.

@nicja
Copy link
Contributor

nicja commented Apr 18, 2019

@mgriego apologies for allowing this to become stale. Could you merge the latest master into this branch? We no longer test against PHP:nightly so the tests should pass then.

@andrefigueira
Copy link

This solves and issue for me, anything I can do to help get the PR moved along?

@kublermdk
Copy link

@nicja are you able to merge this PR?

I've just had to effectively monkey patch my own system to get what I needed.
https://www.kublermdk.com/2022/01/29/googlecloud-flysystem-tweaks-to-support-uniform-bucket-level-access/

I assume others are doing the same given this PR has been sitting around for years.

@nicja
Copy link
Contributor

nicja commented Jan 31, 2022

@kublermdk unfortunately I am no longer an admin on this repo.

@kublermdk
Copy link

@matthewgoslett are you able to merge this PR? It's been sitting here for a long time.
Alternatively can you add others as admins so they can maintain the codebase? Or do you know who has access?

@matthewgoslett
Copy link
Contributor

matthewgoslett commented Jan 31, 2022 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants