flysystem-google-cloud-storage icon indicating copy to clipboard operation
flysystem-google-cloud-storage copied to clipboard

Better object ACL support

Open mgriego opened this issue 7 years ago • 12 comments

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

mgriego avatar May 03 '17 17:05 mgriego

Still need to update the tests...

mgriego avatar May 03 '17 17:05 mgriego

@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 avatar May 03 '17 22:05 mgriego

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

mgriego avatar May 03 '17 22:05 mgriego

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

matthewgoslett avatar May 19 '17 13:05 matthewgoslett

@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 avatar May 22 '17 18:05 mgriego

Same is actually true for the S3 adapter.

mgriego avatar May 22 '17 18:05 mgriego

@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.

nicja avatar Apr 18 '19 14:04 nicja

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

andrefigueira avatar May 13 '20 16:05 andrefigueira

@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.

kublermdk avatar Jan 28 '22 15:01 kublermdk

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

nicja avatar Jan 31 '22 07:01 nicja

@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?

kublermdk avatar Jan 31 '22 07:01 kublermdk

Hey

I'm not admin of this repo and haven't been involved for many years. You'll need to ping someone from the https://github.com/Superbalist organisation. I really don't know anyone who'd be able to assist here.

On Mon, 31 Jan 2022 at 09:29, Michael Kubler @.***> wrote:

@matthewgoslett https://github.com/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?

— Reply to this email directly, view it on GitHub https://github.com/Superbalist/flysystem-google-cloud-storage/pull/57#issuecomment-1025451673, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAL7XH57GGK2U6LBWPEDR5DUYY25XANCNFSM4DJ62VTA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you were mentioned.Message ID: @.*** com>

matthewgoslett avatar Jan 31 '22 07:01 matthewgoslett