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

Check datadirectory owner, not config owner. #27613

Merged
merged 2 commits into from
Feb 28, 2024

Conversation

Dreamsorcerer
Copy link
Contributor

@Dreamsorcerer Dreamsorcerer commented Jun 22, 2021

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()).

@szaimen

This comment has been minimized.

@szaimen szaimen added the 3. to review Waiting for reviews label Jun 23, 2021
@szaimen szaimen added this to the Nextcloud 23 milestone Jun 23, 2021
@skjnldsv skjnldsv requested review from weeman1337 and ChristophWurst and removed request for skjnldsv July 7, 2021 06:24
@ChristophWurst ChristophWurst removed their request for review August 19, 2021 10:39
@skjnldsv skjnldsv mentioned this pull request Oct 13, 2021
@skjnldsv skjnldsv modified the milestones: Nextcloud 23, Nextcloud 24 Oct 22, 2021
@skjnldsv skjnldsv requested review from a team and skjnldsv and removed request for a team October 22, 2021 09:48
@skjnldsv skjnldsv mentioned this pull request Mar 24, 2022
@blizzz blizzz mentioned this pull request Mar 31, 2022
This was referenced Apr 7, 2022
@blizzz blizzz modified the milestones: Nextcloud 24, Nextcloud 25 Apr 21, 2022
This was referenced Aug 12, 2022
This was referenced Aug 24, 2022
@skjnldsv
Copy link
Member

No, we do not squash, we want to preserve the commits history

@nickvergessen nickvergessen removed their request for review February 27, 2024 21:28
cron.php Outdated Show resolved Hide resolved
cron.php Outdated Show resolved Hide resolved
console.php Outdated Show resolved Hide resolved
@skjnldsv skjnldsv force-pushed the patch-2 branch 2 times, most recently from d9a142d to 22ad440 Compare February 28, 2024 12:00
@skjnldsv skjnldsv added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Feb 28, 2024
Copy link
Collaborator

@Altahrim Altahrim left a comment

Choose a reason for hiding this comment

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

🎉

@skjnldsv skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Feb 28, 2024
Signed-off-by: skjnldsv <skjnldsv@protonmail.com>
Signed-off-by: skjnldsv <skjnldsv@protonmail.com>
@Dreamsorcerer
Copy link
Contributor Author

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.

@skjnldsv skjnldsv merged commit 6fc9fec into nextcloud:master Feb 28, 2024
151 of 158 checks passed
@Dreamsorcerer Dreamsorcerer deleted the patch-2 branch February 28, 2024 17:51
@blizzz blizzz mentioned this pull request Mar 5, 2024
@szaimen
Copy link
Contributor

szaimen commented Apr 30, 2024

This breaks e.g. in nextcloud/all-in-one#4542 (comment)

@Dreamsorcerer
Copy link
Contributor Author

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...

@miccgn
Copy link

miccgn commented May 11, 2024

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.

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.

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...

Exactly.

So, how can I disable that check and get OCC back to work?

@Dreamsorcerer
Copy link
Contributor Author

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...

@miccgn
Copy link

miccgn commented May 11, 2024

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.

@szaimen
Copy link
Contributor

szaimen commented May 24, 2024

Hi, we had to revert this pr: #45302
See #45307 on a new approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish enhancement stale Ticket or PR with no recent activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants