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

Fixed an ACL bug which disallowed renaming of files in certain configurations #2792

Merged
merged 1 commit into from
Mar 11, 2024
Merged

Conversation

x7airworker
Copy link
Contributor

@x7airworker x7airworker commented Feb 1, 2024

Steps to reproduce the issue

  1. Create a groupfolder with advanced permissions enabled
  2. Create an ACL configuration which only allows read access to a user/group for the groupfolder.
  3. Create a folder inside the groupfolder and give read/write/create permissions to a user/group.
  4. Create another file inside this folder will full access.

The file created in step 4 can now be deleted and recreated, but not renamed because the delete permission is missing in the parent folder.
The fix I propose checks the element itself or the parent for the required permission. Please let me know if there is a better way or this is a security risk.

@come-nc come-nc requested a review from artonge February 6, 2024 08:54
@come-nc
Copy link
Contributor

come-nc commented Feb 6, 2024

There is a test by @icewind1991 which seems to expect rename to fail in this case.

@icewind1991 Can you review the test and the PR?

Copy link
Contributor

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

@x7airworker
Copy link
Contributor Author

@icewind1991 Kindly bumping this one :)

@icewind1991
Copy link
Member

Nextcloud generally requires write permissions on the parent folder for renames, mirroring the way the permissions work on linux.

Only checking the file could be used to bypass "deny delete" permissions on a folder by moving files outside of the folder before deleting them.

One possible improvement here would be to not require delete permissions (but still require write) on the parent folder if the file is just being renamed, not moved (so target parent and source parent are the same)

@x7airworker
Copy link
Contributor Author

Well I thought that the rename operation is somewhat equivalent to deleting the file and recreating it.
An improvement is definitely needed, because this basically breaks any (useful) folder creation in folders without delete permissions.
Just out of interest: In my example the file to delete is in a parent folder without delete permissions, but with delete permission on the file itself (basically overwriting it). The deletion works just fine, no need for a "deny delete" bypass.

But I think that your proposed implementation would certainly be better.

@x7airworker
Copy link
Contributor Author

@icewind1991 Could you take a look at my changes? I've tested it locally and it seems to work without issues.

@icewind1991
Copy link
Member

approach looks good 👍

@come-nc
Copy link
Contributor

come-nc commented Mar 7, 2024

@x7airworker Some commits are missing the sign-off, can you sign them off or squash them and force push again to fix the DCO ci step?

Signed-off-by: Jake Esser <[email protected]>

Replaced delete permission check with parent comparison

Use strict comparison

Fixed formatting issue

Signed-off-by: Jake Esser <[email protected]>

Formatted code
@x7airworker
Copy link
Contributor Author

@come-nc Done.

@come-nc come-nc merged commit 7544744 into nextcloud:master Mar 11, 2024
43 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants