-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Check datadirectory owner, not config owner. #27613
Conversation
This comment has been minimized.
This comment has been minimized.
No, we do not squash, we want to preserve the commits history |
d9a142d
to
22ad440
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
Signed-off-by: skjnldsv <skjnldsv@protonmail.com>
Signed-off-by: skjnldsv <skjnldsv@protonmail.com>
Thanks for finishing it off, I'm a bit short on time at the moment, so was just making quick updates in the Github UI. |
This breaks e.g. in nextcloud/all-in-one#4542 (comment) |
Why are the users different in this case? The upgrade needs write access to that directory, so seems like it'd make sense to me that it is owned by the same user. Though thinking a little more about that check. Surely it only needs to check that we can write to the directory. I'm not clear why it checked the owner of the file in the first place... |
Having write access to a directory doesn't necessarily mean you need to be the owner. In my case, using NFS, the owner is some UID even not existant on the local www server, but nevertheless www-data gets write access.
Exactly. So, how can I disable that check and get OCC back to work? |
Well, right now, comment out the code. That's what I've had to do for the past 3+ years while waiting for this PR to merge... |
Thanks. Just did so - in both places (cron.php and console.php). Probably a php function like is_writable() would be more appropriate for the task here. |
Changes permission checks to use datadirectory, rather than config file.
Step 1 towards root-owned configs (#11075).
For root-owned configs, the only thing that needs to be writeable is the data directory. For non-root configs, this check shouldn't make any difference, it's very likely the same user is used for both config and data dir.
Currently, the user shouldn't see any difference in behaviour, as the check here would still disallow a root-owned config:
https://github.com/nextcloud/server/blob/master/lib/base.php#L247
Once the rest of the work to support root-owned configs is complete, then this check should be removed (
|| !$configFileWritable && \OCP\Util::needUpgrade()
).