search

silverstripe / silverstripe-assets

Silverstripe Assets component
BSD 3-Clause "New" or "Revised" License
9 stars 66 forks source link

FIX Use 0775 permissions for directories #551

Closed emteknetnz closed 1 year ago

emteknetnz commented 1 year ago

Issue https://github.com/silverstripe/silverstripe-assets/issues/550

michalkleiner commented 1 year ago

My gut feel is that this isn't the best solution, but at the same time I don't have that much time to dig deeper into the original issue and see for myself what could be done differently (if anything) in our code around the case when a file is published and the folder is not changed to be using the dir.public permissions at that point. Also if the permissions are the same as with CMS 4 then it's not that big of a deal.

GuySartorelli commented 1 year ago

I'm on the "if the permissions are the same as with CMS 4 then it's not that big of a deal" side of things - even though it was technically a bug causing those permissions, they're what people have been using for many years now to the extent that those permissions are effectively a feature and now that it's using the "correct" permissions that's causing problems for people.

I think the best solution is, as this PR does, making it so that the CMS 4 behaviour is intentionally kept. Especially since files do follow the more secure behaviour, and ultimately files are what matters in that regard.

emteknetnz commented 1 year ago

Added a unit test to validate the default permissions. Beyond duplicating the config in a unit test I really don't know what else to do.

It's up to league/filesystem to actually apply the permissions to newly created files/folders, so not unit testing that.

GuySartorelli commented 1 year ago

It's up to league/filesystem to actually apply the permissions to newly created files/folders, so not unit testing that.

I'd argue that It's up to us to ensure that our configuration is used to apply those permissions.

It's one thing to validate that the configuration is set which is what your new test does. It's another thing to validate that the configuration is actually used to set the correct permissions based on that configuration. That's what I think we should have here. A test that validates that if the configuration changes, the permissions change to match (or, at the very least, validate that the configuration is correctly passed through to the upstream library, but I think that'll be harder to test and result in a lest robust regression test in any case)

That said, I don't feel super strongly about it - if you give a good reason not to add a test like that I'll be happy to merge.

emteknetnz commented 1 year ago

It's another thing to validate that the configuration is actually used to set the correct permissions based on that configuration.

To me that's very much up to league/flysystem and we shouldn't test that. We should trust that dependencies actually do what they're supposed to do

A test that validates that if the configuration changes, the permissions change to match (or, at the very least, validate that the configuration is correctly passed through to the upstream library

AssetAdapter is a subclass on league/flysystem LocalFilesystemAdapter. We'd be unit testing that we're passing the correct 2nd param to __parent::construct() - https://github.com/silverstripe/silverstripe-assets/blob/2/src/Flysystem/AssetAdapter.php#L67

This seems like it would be a pretty unusual unit test and I don't think we do this elsewhere. I don't think we should do it.

GuySartorelli commented 1 year ago

I still think we should have a test for this - but I don't feel strongly enough about it to stop this PR from being merged, so I'll merge as is.