fix/DAV_permissions_single_read_only_file#54460
fix/DAV_permissions_single_read_only_file#54460tmorgumich wants to merge 10 commits intonextcloud:masterfrom
Conversation
…st.php::testSingleFileReadOnlySharePermissions now green
…g move RE:issue#53041 signed off by tmorg@umich, Troy Morgan
tmorgumich
requested review from
come-nc,
sorbaugh,
susnux,
szaimen and
yemkareems
and removed request for
a team
susnux
left a comment
susnux
left a comment
There was a problem hiding this comment.
Thank you for your pull request, but I think there is a misunderstanding.
In Nextcloud a user share is not the original file but a mount point, meaning e.g. for files you can set permissions like READ or UPDATE but they belong to the content of the file.
The mount point can still be freely renamed by the share receiver - its not the covered by the permissions of the share as those operations are covered by the parent folder permissions - similar to how local filesystems work.
| "require-dev": { | ||
| "bamarni/composer-bin-plugin": "^1.4" | ||
| "bamarni/composer-bin-plugin": "^1.4", | ||
| "phpunit/phpunit": "^10.5" |
There was a problem hiding this comment.
Why? This is already in vendor-bin/phpunit
There was a problem hiding this comment.
I was trying to get phpunit to use the right version, as the autotest.sh was specifying lib/composer/bin/phpunit which I hadn't set up properly(?) and I couldn't run autotest
| @@ -732,6 +732,11 @@ public function rename($source, $target, array $options = []) { | |||
| if ($source == null || $target == null) { | |||
| return false; | |||
| } | |||
| $sourceInfo = $this->getFileInfo($source); | |||
| if ($sourceInfo && !($sourceInfo->getPermissions() & \OCP\Constants::PERMISSION_UPDATE)) { | |||
There was a problem hiding this comment.
Update permission is only for changing the content of a file, moving / copy is set by DELETE / WRITE of the containing folder.
There was a problem hiding this comment.
gotcha, thanks for clarifying.
| $propFind->handle(self::CAN_RENAME_PROPERTYNAME, function () use ( | ||
| $node) { | ||
| return $this->computeCanRename($node) ? 'true' : 'false'; | ||
| }); | ||
|
|
||
| $propFind->handle(self::CAN_MOVE_PROPERTYNAME, function () use ( | ||
| $node) { | ||
| return $this->computeCanMove($node) ? 'true' : 'false'; | ||
| }); | ||
|
|
There was a problem hiding this comment.
Why new properties? We already have the permissions which are assigned on the parent folder.
For copy the node needs READ and the parent needs CREATE permissions.
For move its like copy but with additional parent DELETE permission.
There was a problem hiding this comment.
At first, I definitely misunderstood the issue. I had began to work on a change to FilesPlugin.php which I thought could resolve the misunderstood non-real issue of a recipient being improperly able to move/rename a shared read-only file
|
So if I understand correctly this was a misunderstanding and can be closed? |
|
Hello there, 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! (If you believe you should not receive this message, you can add yourself to the blocklist.) |
4 participants
Summary
I implemented a three line check in lib/private/Files/View.php which aimed at addressing an issue where if Alice shares a file with Bob and sets his permissions to read-only, he is able to go beyond his permissions to the file, which are SGD, missing NV flags:<[oc:permissions]>SGD</oc:permissions>
autotest.sh passes with my added test to ViewTest.php, which creates a recipient and asserts that they are not able to move the file. The style passes tests as well.