Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert "Set umask before operations that create local files" #31293

Closed
wants to merge 9 commits into from
Closed

Revert "Set umask before operations that create local files" #31293

wants to merge 9 commits into from

Conversation

bgottschall
Copy link

@bgottschall bgottschall commented Feb 21, 2022

This reverts the pull request #25280, 'fixing' undescribed issues with "other php stuff". It actually forces folder and file permissions on all files created by nextcloud from within the PHP code which should violate common coding standards in my opinion.

The merge completely ignores the umask from the system that is set by an administrator for various reasons. Just by mentioning one example when someone uses an external folder over the external files plugin, nextcloud will create folder and files there without the group having write permissions which breaks with many NAS setups e.g. Unraid (#29041).

Instead of fixating the servers file and folder permissions within nextcloud ignoring settings carefully chosen by the administrator, the "other php stuff" should be fixed.

Fix #29041

@solracsf solracsf added the 3. to review Waiting for reviews label Feb 21, 2022
@solracsf solracsf changed the title Revert "Merge pull request #25280" Revert "Set umask before operations that create local files" Feb 21, 2022
@solracsf
Copy link
Member

What about #29041 (comment) instead of just reverting? 🤔

@bgottschall
Copy link
Author

bgottschall commented Feb 21, 2022

What about #29041 (comment) instead of just reverting? thinking

This comment only takes the group and other permission portion of the umask and forces the owner umask to 0. While it make sense from the nextcloud perspective to make sure that it always has read/write permissions on the files it creates it is a unecessary change to begin with. A process umask is set by whoever starts it (systemctl service e.g.) which is typically 022.
If a system administrator chooses a different mask he should be allowed to and nextcloud should not change it. If the administrator chooses a mask that crashes with nextcloud, it is a configuration problem and it should not be nextcloud task to alleviate this potential wrong doing. One could add in the adminstrator documentation that nextcloud expects a umask that preserves read/write permissions of files and folders, but hardcoding this safeguard into the php code?

Edit: And I want to add that it is totally unclear what this merge fixed in the first place.

@bgottschall
Copy link
Author

bgottschall commented Mar 7, 2022

I've removed the chmod inside mkdir, which is a line that is also not necessary.

Referring to the PHP doc https://www.php.net/manual/en/function.mkdir.php mkdir respects the umask of the system, meaning the folders are created with the 755 permissions if the umask is 022 for example. Another aspect is that mkdir is called with recursive set to true. Setting only the last directory using chmod is incomplete as all parent directories that might have been created would not be affected.

Copy link
Contributor

@come-nc come-nc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could add a check in https://github.com/nextcloud/server/blob/master/apps/settings/lib/Controller/CheckSetupController.php that checks the umask? But maybe this is not possible if umask can be set differently in each storage or folder, not sure.

tests/lib/Files/Storage/LocalTest.php Outdated Show resolved Hide resolved
@bgottschall
Copy link
Author

Maybe we could add a check in https://github.com/nextcloud/server/blob/master/apps/settings/lib/Controller/CheckSetupController.php that checks the umask? But maybe this is not possible if umask can be set differently in each storage or folder, not sure.

The umask is a process configuration (not folder or storage related), however the process can change its umask at any given time. That means php code (apps/extensions) could change the umask simply with umask(...). I don't see a reason why they ever should do that and if, they should fix it there or revert it back to the process default after being finished.
Though if one changes the umask in the php code, it will only be changed for the local execution context and the next request starts with the process default again.

@come-nc
Copy link
Contributor

come-nc commented Mar 7, 2022

Maybe we could add a check in https://github.com/nextcloud/server/blob/master/apps/settings/lib/Controller/CheckSetupController.php that checks the umask? But maybe this is not possible if umask can be set differently in each storage or folder, not sure.

The umask is a process configuration (not folder or storage related), however the process can change its umask at any given time. That means php code (apps/extensions) could change the umask simply with umask(...). I don't see a reason why they ever should do that and if, they should fix it there or revert it back to the process default after being finished. Though if one changes the umask in the php code, it will only be changed for the local execution context and the next request starts with the process default again.

So if it is the same for the whole process we can test it in CheckSetupController and warn if it is a mask that will cause problem (if it will disallow editing what we create). Ideally with a link to the documentation about this.

@bgottschall
Copy link
Author

Maybe we could add a check in https://github.com/nextcloud/server/blob/master/apps/settings/lib/Controller/CheckSetupController.php that checks the umask? But maybe this is not possible if umask can be set differently in each storage or folder, not sure.

The umask is a process configuration (not folder or storage related), however the process can change its umask at any given time. That means php code (apps/extensions) could change the umask simply with umask(...). I don't see a reason why they ever should do that and if, they should fix it there or revert it back to the process default after being finished. Though if one changes the umask in the php code, it will only be changed for the local execution context and the next request starts with the process default again.

So if it is the same for the whole process we can test it in CheckSetupController and warn if it is a mask that will cause problem (if it will disallow editing what we create). Ideally with a link to the documentation about this.

Added a check, would appreciate if somebody could take over the frontend including testing as my setup doesn't allows testing currently and I haven't touched anything on the frontend yet.

Björn Gottschall added 8 commits March 18, 2022 15:40
…sions"

This reverts commit f3738ee, reversing
changes made to 0a6416f.

Signed-off-by: Björn Gottschall <github.mail@bgottschall.de>
Signed-off-by: Björn Gottschall <github.mail@bgottschall.de>
Signed-off-by: Björn Gottschall <github.mail@bgottschall.de>
Signed-off-by: Björn Gottschall <github.mail@bgottschall.de>
Signed-off-by: Björn Gottschall <github.mail@bgottschall.de>
Signed-off-by: Björn Gottschall <github.mail@bgottschall.de>
@skjnldsv skjnldsv mentioned this pull request Mar 24, 2022
@blizzz blizzz mentioned this pull request Mar 31, 2022
@blizzz blizzz mentioned this pull request Apr 7, 2022
@bgottschall
Copy link
Author

A bit unfortunate that this is not moving forward. I mean there is a missing frontend message of the umask server check but this pull request is mainly about removing faulty code not introducing something new in the server.

@PVince81
Copy link
Member

obsoleted by #32723

@PVince81 PVince81 closed this Jun 10, 2022
@szaimen szaimen removed this from the Nextcloud 25 milestone Jun 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews
Projects
None yet
6 participants