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

Set umask before operations that create local files #25280

Merged
merged 1 commit into from
Mar 30, 2021

Conversation

icewind1991
Copy link
Member

@icewind1991 icewind1991 commented Jan 22, 2021

this solves issues where "other php stuff" is messing with the umask

@icewind1991 icewind1991 added the 2. developing Work in progress label Jan 22, 2021
@PVince81
Copy link
Member

does this fix any known issue ?
could this have side effects ?

@rullzer
Copy link
Member

rullzer commented Mar 22, 2021

What is the status here?

@icewind1991 icewind1991 force-pushed the explicit-file-permissions branch from 93afb8d to e09921f Compare March 23, 2021 13:34
this solves issues where "other php stuff" is messing with the umask

Signed-off-by: Robin Appelman <[email protected]>
@icewind1991 icewind1991 force-pushed the explicit-file-permissions branch from e09921f to e5dc1a8 Compare March 23, 2021 13:52
@icewind1991 icewind1991 changed the title explicitly set permissions for newly created files Set umask before operations that create local files Mar 23, 2021
@icewind1991 icewind1991 added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Mar 23, 2021
@icewind1991 icewind1991 requested review from rullzer and PVince81 March 23, 2021 15:58
@icewind1991
Copy link
Member Author

This should be good for review now.

This solves a case where some unknown external "thing" was messing with the umask, causing files and folders created by nextcloud to not be writable

@PVince81
Copy link
Member

PVince81 commented Mar 24, 2021

using this PR to retest CI after nextcloud/docker-ci#274
curious to see if #26265 was really necessary

CI restarted

Copy link
Member

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

👍 code looks fine

@rullzer rullzer added this to the Nextcloud 22 milestone Mar 30, 2021
@rullzer rullzer merged commit f3738ee into master Mar 30, 2021
@rullzer rullzer deleted the explicit-file-permissions branch March 30, 2021 19:55
@icewind1991
Copy link
Member Author

/backport to stable21

@icewind1991
Copy link
Member Author

/backport to stable20

@PVince81
Copy link
Member

PVince81 commented Nov 4, 2021

seems this has some side effect: #29041

@bgottschall
Copy link

This merge is quite unfortunate for the Unraid community, which uses the umask for Nextcloud to change the default permissions of files and folders created since they are working with docker mounts that require the group permissions to have write access.

You probably don't care about such scenarios where Nextcloud is working together with smb shares, but in any case I consider setting fixed permissions inside the PHP code as bad practice. I would very much like to see a configuration parameter here to change them.

@tatankat
Copy link

tatankat commented Feb 9, 2022

The issues mentioned should be fixed by fixing the "other php stuff" (whatever it may be...), as they are the cause of the problem. Not by hiding the real issues by forcefully changing a carefully admin-selected sensible umask in Nextcloud, which breaks stuff and makes Nextcloud part of the issue.
As @bgottschall, I consider this bad practice, but I think a configuration parameter is not really required as it can already be configured server wide.

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
Development

Successfully merging this pull request may close these issues.

5 participants