October: Unable to pass S3 ACL setting

[Floriskoch]

Expected behavior

Expected to be able to set an ACL variable when using AWS S3 as filesystem. By default this is set to private but some of our client buckets require us to set this to public on each individual file.

Actual behavior

There is no way to set the ACL variable within OctoberCMS. To make this work I have to change the core files of OctoberCMS.

Reproduce steps

A way to force this to work is changing the following in MediaLibrary.php:

public function put($path, $contents)
{
    $path = self::validatePath($path);
    $fullPath = $this->getMediaPath($path);
    return $this->getStorageDisk()->put($fullPath, $contents);
}

to, adding a third argument to the put method of the storage disk:

public function put($path, $contents)
{
    $path = self::validatePath($path);
    $fullPath = $this->getMediaPath($path);
    return $this->getStorageDisk()->put($fullPath, $contents, 'public');
}

It would be great if this setting could be added to config/filesystems.php.

October build

419

[LukeTowers]

Is the third parameter passed to $this->getStorageDisk()->put() documented anywhere? While it works for your S3 disk, I'm not sure what the effect a blanket inclusion of that parameter would have on every other type of storage driver out there. I feel like this is something that needs to get configured on a disk level, not provided on each method call to the disk.

[harmenjanssen]

As team-member to @Floriskoch I'd like to elaborate: I agree this should be a very specific S3 setting, not something that's used for every type of disk.
Not in the least because there are a lot of other useful headers you can sent along to the S3 API.
Content-Encoding (for gzip) and Cache-control being among the most useful.

So any kind of hook that would allow setting these headers, specifically for Amazon S3, would be what we're after.

[LukeTowers]

Well, if we controlled those within the filesystem config, then you could control it programmatically with Config::set('filesystems.s3.example', 'value');. Does that make sense in accordance with how you would use them, or what do you envision sending those headers to Amazon S3 looking like? How is that normally handled in just a plain-Laravel setting?

[harmenjanssen]

Hmm. For a lot of settings that would be great, but I suspect sooner or later we would like to do some per-file configuration.
I can imagine specific cache lifetime per file, and I actually remember from a past project that we specifically turned _off_ gzip for audio files, because that turned out to cause bugs in Safari.

If 'value' in your example could optionally be a callback function, that would solve everything I guess.

// Not sure which arguments could realistically be passed to this function
Config::set('filesystems.s3.example', function($filename, $bytes) {
  return $fileSpecificHeader;
});

Note that I'm not really experienced with Laravel so this might not make a lot of sense.

[LukeTowers]

Unfortunately not, the Config facade is essentially a key value store that's initialized from the config files but can be modified at runtime. I proposed it because then we don't have to do anything else, we just have to make sure that the Amazon S3 storage driver supports receiving that information from the config store. At that point, you could then hook into more specific events to selectively change certain values within the config store to achieve the behaviour you are looking for.

[Floriskoch]

To me this seems like a good solution for now! I'm curious though what you mean with "more specific events" in your last sentence. Are you talking about OctoberCMS's systemEvents or something else?

[LukeTowers]

I mean the events listed on my spreadsheet that will eventually be fully documented in the API documentation.

So, you would do something like this:

// Note, this event might actually be too late to properly hook into what you need,
// but that can be resolved later
Event::listen('media.file.upload', function($mediaManager, $filePath, $uploadedFile) {
    if ($file) { // some matching condition
        Config::set('filesystems.disks.s3.headers.key', 'value');
    }
});

[harmenjanssen]

Sounds good, @LukeTowers.
I sort of feel a bit icky about updating a global variable for every upload and just leaving it in whatever state the last updated file wanted it to be, but I guess for our purposes it's fine and would not cause a lot of trouble.

Is there anything we can do to help to implement this?

[LukeTowers]

Config::set() doesn't persist the changes across requests, it only makes that change for that request's lifetime.

If you wanted to be super clean about it, you could do something like:

$s3ConfigKey = 'filesystems.disks.s3.headers.key';
// note: event doesn't actually exist yet
Event::listen('media.file.beforeUpload', function($mediaManager, $filePath, $uploadedFile) use ($s3ConfigKey) {
    // Temporarily store the original configuration for later restoration
    Config::set('myvendor.myplugin.temp.s3Key', Config::get($s3ConfigKey));
    // Stub: Evaluate and make the changes you need to make to the S3 config for this file upload
});

Event::listen('media.file.upload', function($mediaManager, $filePath, $uploadedFile) use ($s3ConfigKey) {
    // Restore the original state
    Config::set($s3ConfigKey, Config::get('myvendor.myplugin.temp.s3Key'));
});

And you don't even necessarily need to temporarily store the original value within a Config keyvalue store, it's just the first thing that popped into my head for easy use in this situation.

Essentially, the problem of global state change for every upload is mitigated by
a. The state only changes at a request level, it doesn't persist across requests
b. You can always change it back later on in the request if you would like to be extra safe.

As far as what you can do to help make this a reality, look into the Laravel S3 filesystem driver and figure out the official ways that the driver supports sending those headers. If we can get that driver looking at the disk config for those values, then we're pretty much 90% of the way there.

[harmenjanssen]

Yeah, I guess you're right about the global config.

I'll see what we can dig up on the Laravel S3 filesystem. Thanks for brainstorming with us on this 🙂

[LukeTowers]

Not a problem, hope you guys find a solution. Keep me updated with whatever solution you eventually go with!

[HammenWS]

For setting the ACL for the s3 disk you can use the following option in config\filesystems.php.

's3' => [
    'driver' => 's3',
    ...
    'visibility' => 'public'
],

I still need to find a way to give extra configuration options like request headers. Working on it...

[Floriskoch]

This is resolved. Apparently it's as easy as adding a visibility property to the config file, which is great! @LukeTowers Maybe add this to the documentation? I can imagine more people will be looking for this :-)

[LukeTowers]

You're welcome to add it to the docs yourself, personally I'm not sure where it makes sense including it. Take a look at https://github.com/octobercms/docs and see if you can create a PR to add it to the documentation.

[colthreepv]

It's really key for some projects putting a default request headers for the entire configured filesystem (S3 bucket, actually).
Amazon after years of polishing still does not allow to have header defaults, so we are obliged to specify request headers to each PUT request.

@HammenWS Is there any workaround currently working? Or something that can be done about putting a simple default