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

Expose umask override value as config parameter: localstorage.umask #32723

Merged
merged 1 commit into from
Jun 10, 2022

Conversation

MartinBrugnara
Copy link
Contributor

@MartinBrugnara MartinBrugnara commented Jun 3, 2022

Commit #451c06d introduced override for umask value.
This is needed to avoid broken env configuration or dirty workers
to mess with the permissions when creating new files.

Most Nextcloud, that does not integrate with external software
would work fine with an hard-coded value (#451c06d set it at 022).

Advanced install may require more flexibility, as such this commit
exposes the "umask override value" as configuration parameter:
localstorage.umask

It defaults to 0022 both in code and in config/config.sample.php .

extra: I feel like a better default umask should be 7022, but that value breaks some tests and builds. I propose to postpone this discussion 0022 vs 7022 to another issue/pr.

@kesselb
Copy link
Contributor

kesselb commented Jun 5, 2022

Hi, please revert the whitespace changes.

lib/private/Files/Storage/Local.php Fixed Show fixed Hide fixed
lib/private/Files/Storage/Local.php Fixed Show fixed Hide fixed
lib/private/Files/Storage/Local.php Fixed Show fixed Hide fixed
lib/private/Files/Storage/Local.php Fixed Show fixed Hide fixed
lib/private/Files/Storage/Local.php Fixed Show fixed Hide fixed
lib/private/Files/Storage/Local.php Fixed Show fixed Hide fixed
lib/private/Files/Storage/Local.php Fixed Show fixed Hide fixed
lib/private/Files/Storage/Local.php Fixed Show fixed Hide fixed
lib/private/Files/Storage/Local.php Fixed Show fixed Hide fixed
lib/private/Files/Storage/Local.php Fixed Show fixed Hide fixed
Commit 451c06d introduced override for umask value.
This is needed to avoid broken env configuration or dirty workers
to mess with the permissions when creating new files.

Most Nextcloud, that does not integrate with external software
would work fine with an hard-coded value (451c06d set it at 022).

Advanced install may require more flexibility, as such this commit
exposes the "umask override value" as configuration parameter:
`localstorage.umask`

It defaults to 0022 both in code and in config/config.sample.php .

Signed-off-by: Martin Brugnara <[email protected]>
@MartinBrugnara
Copy link
Contributor Author

@kesselb fixed commit (and .vimrc), updated commit message to reflect 0022 vs 7022 and force pushed.

@MartinBrugnara
Copy link
Contributor Author

May I kindly ask help understanding the broken tests?

1. ci/drone/pr: says is failing on acceptance-app-files-sharing-link, but the test summary reports no failures: 14 scenarios (13 passed, 1 skipped). What am I missing?
2. S3 primary storage / php8.0-objectstore-minio: fails connecting to S3 via MinIO local proxy.
3. S3 primary storage / s3-primary-summary: it's failing because it has a hard-dep on the previous step/issue.

About 1, I cannot see which is the step that is actually failing. Again, 13 passed and 1 skipped. Where are the failures?

About 2 and 3, it seems to be just a transient error or anyway, nothing related to this PR. Could please someone with auth try to re-issue the test?

@kesselb
Copy link
Contributor

kesselb commented Jun 5, 2022

May I kindly ask help understanding the broken tests?

1. ci/drone/pr: says is failing on acceptance-app-files-sharing-link, but the test summary reports no failures: 14 scenarios (13 passed, 1 skipped). What am I missing? 2. S3 primary storage / php8.0-objectstore-minio: fails connecting to S3 via MinIO local proxy. 3. S3 primary storage / s3-primary-summary: it's failing because it has a hard-dep on the previous step/issue.

About 1, I cannot see which is the step that is actually failing. Again, 13 passed and 1 skipped. Where are the failures?

About 2 and 3, it seems to be just a transient error or anyway, nothing related to this PR. Could please someone with auth try to re-issue the test?

Should be fine to ignore it for now.

@kesselb kesselb added enhancement 3. to review Waiting for reviews labels Jun 5, 2022
@kesselb kesselb added this to the Nextcloud 25 milestone Jun 5, 2022
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.

👍

@PVince81 PVince81 merged commit bea755f into nextcloud:master Jun 10, 2022
@welcome
Copy link

welcome bot commented Jun 10, 2022

Thanks for your first pull request and welcome to the community! Feel free to keep them coming! If you are looking for issues to tackle then have a look at this selection: https://github.com/nextcloud/server/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22

@PVince81
Copy link
Member

/backport to stable24

@PVince81
Copy link
Member

/backport to stable23

@PVince81
Copy link
Member

/backport to stable22

@backportbot-nextcloud
Copy link

The backport to stable23 failed. Please do this backport manually.

@backportbot-nextcloud
Copy link

The backport to stable24 failed. Please do this backport manually.

@backportbot-nextcloud
Copy link

The backport to stable22 failed. Please do this backport manually.

@david-kalbermatten
Copy link

Will this ever get backported to version 24? I implemented my own fix using $_SERVER['UMASK'] to read out the ENV_VAR from the Docker container, but this fix would work more universally.

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 enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants