FIX Permissions when writing export folders

[JoshuaCarter]

Solves: #36

[NightJar]

Relevant: https://www.computerhope.com/unix/uumask.htm

My understanding of this change is that it is basically chmod 777 $FILE

http://php.net/umask

Specifically:

Note:

Avoid using this function in multithreaded webservers. It is better to change the file permissions with chmod() after creating the file. Using umask() can lead to unexpected behavior of concurrently running scripts and the webserver itself because they all use the same umask.

In short - I'm not sure this is a safe change.

[NightJar]

Perhaps it could be altered to use chmod as suggested by the note after writing?
That way making files generically executable would also help keep things a little more secure :)

[JoshuaCarter]

@NightJar Glad someone knows what they're doing 🙈. Change made :)

[NightJar]

Haha, not sure I'd go as far as to say I know what I'm doing... I'm just reviewing the change & researching the impacts ;)

Having seen the newer change - it looks like the folders are already created as 770, giving user and group read, write, and execute access. Perhaps it's the final bit that needs changing (other)
The chmod seems redundant, you should be able to change the second parameter in the mkdir call... although it does mention that the umask can affect this. I might be misunderstanding something in how umask affects things (highly likely)... maybe chmod is still needed 🤷‍♂️

Are you able to confirm that this latest change resolves your issue still?

It's probably also worth noting that SS4 uses a file abstraction layer - direct file system calls like this might not be the best idea (e.g. if the assets are all remotely stored on an S3 bucket or something).

While this change is certainly a thing that's needed, the 'fix' might unfortunately be a bit more complex than a simply permission mask manipulation.

But if this works, then that last part can certainly be it's own ticket issue :)

[NightJar]

Sorry that was a bit of a hectic brain dump of a comment.

TL;DR: if you can confirm this patch still works for you, I'll merge it :)

[JoshuaCarter]

@NightJar No stress. I confirmed it was working prior to pinging you. In our setup, the user mask is 022, so when masked with 770, our perms come out as 750 which is the expected result of mkdir. When we add chmod(0770), the perms come out as 770, because the user mask doesn't influence chmod.

The only relevent involvement SS seems to have (which I'm looking into soon) is that our ansible profile actually defines the umask as 002, and then something overwrites that to 022. Technically, if we fixed that in our env, we wouldn't need this PR. That said, with the way this module works, I think it's better to assume that group will need write perms, but I could also understand disagreeing with that.

In short, this change would make the module work more reliably, but also negate any effect from the user mask.

Ultimately your call :)

[NightJar]

Thanks for that extra context @JoshuaCarter, most excellent!

One more very minor request from me, then given this extra context I may refer with some colleagues before actually merging, sorry! I think it should be OK though, but that caveat of negating user mask (albeit in this use case only) is worthy of a second opinion I think :)

My request is only this:
Could you please summarise that last comment nicely into the actual commit message (via --amend or what-have-you)? It is very relevant information to this commit, and having prior experience in ops I can say it makes the world of difference when debugging an issue to know why a change was made, not only what it does (which one can read in the code diff anyway). Most particularly that last part with the caveat about umask!

Cheers!

[robbieaverill]

In terms of your umask problem, we could add extension points if you'd like - then your user code could adjust the masks.

The real fix for this is to use the asset abstraction layer though.

[JoshuaCarter]

@robbieaverill Extension points are a workable solution too. In our case, we figured out that cron wasn't loading a user's .profile where the mask (in this case, 002) is declared, and we've found a work-around.

I'm not familiar enough with the SS asset abstraction layer to know if it uses it's own mask, etc. If so, it's a potential solution, and if not, then it would have no impact here.

At this point, whether this change should be made is very much up to you guys. I've altered the commit message (as requested by @NightJar ) so I'll leave it in your hands to decide whether to merge or not. Though I will point out that a couple extension points would be the 'everyone's happy' solution. I'm happy to alter my change in that direction if that's what you decide :)

[NightJar]

Having had a better think about this... I think it's safe to merge if we feature flag it. Relying on the umask to behave as expected is probably a desired trait of the system.
To get around this I propose 2 changes:

1. Add a config variable ignore_umask (boolean) and gate the chmod call on whether or not it evaluates to true. Default this to false so current behaviour is maintained.
2. (optional) Add another config variable permission_mask (or a more apt name if you can think of one) - and have this used as both the parameter to mkdir and chmod. The format should be an octal form, as it is now (which may or may not require double checking first, particularly if it's read from a string).

A comment about why the chmod is necessary (i.e. that mkdir obeys umask, and chmod sets a definite permission) would also be very cool.

As already mentioned the better fix would be to use the file system abstraction, but that can be a different PR. I do not wish to block contribution based on architectural decisions above what a PR is trying to achieve :)

[JoshuaCarter]

@NightJar Done and tested. Let me know if any changes need to be made.

The most questionable choice I made was to force the permission_mode to be a string value.
This is because "0770" and "770" will both produce the same output when treated as octal values. But 0770 and 770 will produce differing values when treated as octals, etc.

I'd love if we could get this into a feature/patch release sometime soon if possible as my previously mentioned work-around didn't pan out as hoped, and so this would be a great work-around until we can better sort out our server environment, etc.

Cheers :)

[NightJar]

Thanks for this @JoshuaCarter :)
Good follow ups, nice docblocks!

[JoshuaCarter]

@NightJar great, thanks! Any idea when this will go out in a release?

[NightJar]

@JoshuaCarter How about now? :)
https://github.com/silverstripe/silverstripe-gridfieldqueuedexport/releases/tag/2.2.0

[JoshuaCarter]

@NightJar Great, thanks :)