Storage: copy_blob doesn't respect preserve_acl about python-storage HOT 11 CLOSED

I'm okay with not introducing support for copying ACL in this case. It reduces potential security risks.

However, I do see the potential for having a destination_blob with metadata parameter being passed in. That would help in this case.

from python-storage.

Apologies for the delay.

I confirmed with the GCS team that "ACL being copied" behavior was never supported by the copy API in GCS. This might have just been an accident.

I prefer we deprecate in this case. I'd prefer to have a user do a subsequent ACL update in this case or that we have a FR to introduce a parameter to supply metadata when using copy_blob().

from python-storage.

I really wonder how an argument gets added if it never had an implementation.. But this bug also makes the very existence of rename_blob very misleading, unless it is extended to also include copying of the ACL, because that is definitely what one would expect from a function with that name.

Ultimately it would make much more sense if GCS would just add proper move and copy operations in the API though, but not sure where to even ask for that.

from python-storage.

@frankyn Is this the expected behavior for preserve_acl?

from python-storage.

Taking a look, thanks for the ping.

from python-storage.

Hi folks,

Based on the code, https://github.com/googleapis/google-cloud-python/blob/a79d98de3e9d6890c4e98d33cecae2f80550fdfd/storage/google/cloud/storage/bucket.py#L1121

I don't see anything necessarily copying ACLs. When exercising the example provided by @HenrikOssipoff, It's not something Cloud Storage does on a copy request.

We may want to deprecate this parameter.

from python-storage.

I agree with @frankyn. Let's mark this deprecated.

from python-storage.

The objects.copy API actually takes an optional Object resource in the request body, and applies any metadata to the destination object. Maybe we need to use pass the source blob's properties (including ACL, which we normally don't pass)?

from python-storage.

@frankyn We currently seem to promise that the ACL will be copied unless the user passes preserve_acl=False. Can you check whether the backend changed the "copy ACL" behavior, and if so, when?

Alternatives:

• Follow up on that promise by passing the source blob's ACL along in the body of the POST request.
• Deprecate the preserve_acl flag properly, preserving the current "erase ACL if False" behavior for a certain number of releases. We should also emit a warning if the user passes an explicit True value, since the backend doesn't copy (or no longer copies) the ACL.

For clarity, we could also add a Blob.copy_from method which passes along the destination blob's metadata / ACL (or at least any of the changed metadata?) in the body of the POST request. E.g.:

source_blob = bucket.get_blob("source-blob-name")
destination_blob = bucket.blob("destination-blob-name")
destination_blob.acl.all().grant_read()
destination_blob.storage_class = "COLDLINE"
destination_blob.copy_from(source_blob)  # passes ACL, storage class in body

from python-storage.

For context, see googleapis/google-cloud-python#5456.

from python-storage.

For future reference, this is the function I am using to move a file now. I noticed that all metadata seems to be copied correctly, but it does not cover everything, for example the time_created gets bumped here.

def move(source: str, destination: str):
    source_blob = media_bucket.blob(source)
    destination_blob = media_bucket.copy_blob(
        blob=source_blob,
        destination_bucket=media_bucket,
        new_name=destination,
    )
    destination_blob.acl.save(acl=source_blob.acl)
    source_blob.delete()

from python-storage.