From e0a566ca1d3cd3aea70c54060f70b197f0155768 Mon Sep 17 00:00:00 2001 From: Ferdinand Thiessen Date: Thu, 28 Aug 2025 00:35:19 +0200 Subject: [PATCH 1/2] fix(dav): ensure moving or copying a file is possible Signed-off-by: Ferdinand Thiessen --- apps/dav/lib/Connector/Sabre/SharesPlugin.php | 52 ++++++++- .../sharing_features/sharing-v1-part4.feature | 107 ++++++++++++++++++ 2 files changed, 158 insertions(+), 1 deletion(-) diff --git a/apps/dav/lib/Connector/Sabre/SharesPlugin.php b/apps/dav/lib/Connector/Sabre/SharesPlugin.php index d1cadee5f8bd0..78d16f5d40ac3 100644 --- a/apps/dav/lib/Connector/Sabre/SharesPlugin.php +++ b/apps/dav/lib/Connector/Sabre/SharesPlugin.php @@ -29,13 +29,16 @@ namespace OCA\DAV\Connector\Sabre; use OC\Share20\Exception\BackendError; +use OCA\DAV\Connector\Sabre\Exception\Forbidden; use OCA\DAV\Connector\Sabre\Node as DavNode; use OCP\Files\Folder; use OCP\Files\Node; use OCP\Files\NotFoundException; +use OCP\Files\Storage\ISharedStorage; use OCP\IUserSession; use OCP\Share\IManager; use OCP\Share\IShare; +use Sabre\DAV\Exception\NotFound; use Sabre\DAV\PropFind; use Sabre\DAV\Server; use Sabre\DAV\Tree; @@ -88,7 +91,9 @@ public function initialize(Server $server) { $server->protectedProperties[] = self::SHAREES_PROPERTYNAME; $this->server = $server; - $this->server->on('propFind', [$this, 'handleGetProperties']); + $this->server->on('propFind', $this->handleGetProperties(...)); + $this->server->on('beforeCopy', $this->validateMoveOrCopy(...)); + $this->server->on('beforeMove', $this->validateMoveOrCopy(...)); } /** @@ -225,4 +230,49 @@ public function handleGetProperties( return new ShareeList($shares); }); } + + /** + * Ensure that when copying or moving a node it is not transferred from one share to another, + * if the user is neither the owner nor has re-share permissions. + * For share creation we already ensure this in the share manager. + */ + public function validateMoveOrCopy(string $source, string $target): bool { + try { + $targetNode = $this->tree->getNodeForPath($target); + } catch (NotFound) { + [$targetPath,] = \Sabre\Uri\split($target); + $targetNode = $this->tree->getNodeForPath($targetPath); + } + + $sourceNode = $this->tree->getNodeForPath($source); + if ((!$sourceNode instanceof DavNode) || (!$targetNode instanceof DavNode)) { + return true; + } + + $sourceNode = $sourceNode->getNode(); + if ($sourceNode->isShareable()) { + return true; + } + + $targetShares = $this->getShare($targetNode->getNode()); + if (empty($targetShares)) { + // Target is not a share so no re-sharing inprogress + return true; + } + + $sourceStorage = $sourceNode->getStorage(); + if ($sourceStorage->instanceOfStorage(ISharedStorage::class)) { + // source is also a share - check if it is the same share + + /** @var ISharedStorage $sourceStorage */ + $sourceShare = $sourceStorage->getShare(); + foreach ($targetShares as $targetShare) { + if ($targetShare->getId() === $sourceShare->getId()) { + return true; + } + } + } + + throw new Forbidden('You cannot move a non-shareable node into a share'); + } } diff --git a/build/integration/sharing_features/sharing-v1-part4.feature b/build/integration/sharing_features/sharing-v1-part4.feature index baffe0eca0130..893f5b3eeeb83 100644 --- a/build/integration/sharing_features/sharing-v1-part4.feature +++ b/build/integration/sharing_features/sharing-v1-part4.feature @@ -180,3 +180,110 @@ Scenario: publicUpload overrides permissions | uid_file_owner | user0 | | share_type | 3 | | permissions | 1 | + +Scenario: Cannot copy files from share without share permission into other share + Given user "user0" exists + Given user "user1" exists + Given user "user2" exists + And As an "user0" + And user "user0" created a folder "/share" + When creating a share with + | path | share | + | shareType | 0 | + | shareWith | user1 | + | permissions | 15 | + Then the HTTP status code should be "200" + And the OCS status code should be "100" + And User "user0" uploads file with content "test" to "/share/test.txt" + And As an "user1" + And user "user1" created a folder "/re-share" + When creating a share with + | path | re-share | + | shareType | 0 | + | shareWith | user2 | + | permissions | 31 | + Then the HTTP status code should be "200" + And the OCS status code should be "100" + When User "user1" copies file "/share/test.txt" to "/re-share/copytest.txt" + Then the HTTP status code should be "403" + +Scenario: Cannot move files from share without share permission into other share + Given user "user0" exists + Given user "user1" exists + Given user "user2" exists + And As an "user0" + And user "user0" created a folder "/share" + When creating a share with + | path | share | + | shareType | 0 | + | shareWith | user1 | + | permissions | 15 | + Then the HTTP status code should be "200" + And the OCS status code should be "100" + And User "user0" uploads file with content "test" to "/share/test.txt" + And As an "user1" + And user "user1" created a folder "/re-share" + When creating a share with + | path | re-share | + | shareType | 0 | + | shareWith | user2 | + | permissions | 31 | + Then the HTTP status code should be "200" + And the OCS status code should be "100" + When User "user1" moves file "/share/test.txt" to "/re-share/movetest.txt" + Then the HTTP status code should be "403" + +Scenario: Cannot move folder containing share without share permission into other share + Given user "user0" exists + Given user "user1" exists + Given user "user2" exists + And As an "user0" + And user "user0" created a folder "/share" + When creating a share with + | path | share | + | shareType | 0 | + | shareWith | user1 | + | permissions | 15 | + Then the HTTP status code should be "200" + And the OCS status code should be "100" + And User "user0" uploads file with content "test" to "/share/test.txt" + And As an "user1" + And user "user1" created a folder "/contains-share" + When User "user1" moves file "/share" to "/contains-share/share" + Then the HTTP status code should be "201" + And user "user1" created a folder "/re-share" + When creating a share with + | path | re-share | + | shareType | 0 | + | shareWith | user2 | + | permissions | 31 | + Then the HTTP status code should be "200" + And the OCS status code should be "100" + When User "user1" moves file "/contains-share" to "/re-share/movetest" + Then the HTTP status code should be "403" + +Scenario: Can copy file between shares if share permissions + Given user "user0" exists + Given user "user1" exists + Given user "user2" exists + And As an "user0" + And user "user0" created a folder "/share" + When creating a share with + | path | share | + | shareType | 0 | + | shareWith | user1 | + | permissions | 31 | + Then the HTTP status code should be "200" + And the OCS status code should be "100" + And User "user0" uploads file with content "test" to "/share/test.txt" + And As an "user1" + And user "user1" created a folder "/re-share" + When creating a share with + | path | re-share | + | shareType | 0 | + | shareWith | user2 | + | permissions | 31 | + Then the HTTP status code should be "200" + And the OCS status code should be "100" + When User "user1" copies file "/share/test.txt" to "/re-share/movetest.txt" + Then the HTTP status code should be "201" From 6f189cc034b48d0c9a653fca99d654b59877077f Mon Sep 17 00:00:00 2001 From: Ferdinand Thiessen Date: Tue, 2 Sep 2025 19:09:30 +0200 Subject: [PATCH 2/2] chore: adjust for legacy Nextcloud 29 and before (use OCA instead of OCP) Signed-off-by: Ferdinand Thiessen --- apps/dav/lib/Connector/Sabre/SharesPlugin.php | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/apps/dav/lib/Connector/Sabre/SharesPlugin.php b/apps/dav/lib/Connector/Sabre/SharesPlugin.php index 78d16f5d40ac3..803b981f5b313 100644 --- a/apps/dav/lib/Connector/Sabre/SharesPlugin.php +++ b/apps/dav/lib/Connector/Sabre/SharesPlugin.php @@ -31,10 +31,11 @@ use OC\Share20\Exception\BackendError; use OCA\DAV\Connector\Sabre\Exception\Forbidden; use OCA\DAV\Connector\Sabre\Node as DavNode; +use OCA\Files_Sharing\ISharedStorage; +use OCA\Files_Sharing\SharedStorage; use OCP\Files\Folder; use OCP\Files\Node; use OCP\Files\NotFoundException; -use OCP\Files\Storage\ISharedStorage; use OCP\IUserSession; use OCP\Share\IManager; use OCP\Share\IShare; @@ -91,9 +92,9 @@ public function initialize(Server $server) { $server->protectedProperties[] = self::SHAREES_PROPERTYNAME; $this->server = $server; - $this->server->on('propFind', $this->handleGetProperties(...)); - $this->server->on('beforeCopy', $this->validateMoveOrCopy(...)); - $this->server->on('beforeMove', $this->validateMoveOrCopy(...)); + $this->server->on('propFind', [$this, 'handleGetProperties']); + $this->server->on('beforeCopy', [$this, 'validateMoveOrCopy']); + $this->server->on('beforeMove', [$this, 'validateMoveOrCopy']); } /** @@ -264,7 +265,7 @@ public function validateMoveOrCopy(string $source, string $target): bool { if ($sourceStorage->instanceOfStorage(ISharedStorage::class)) { // source is also a share - check if it is the same share - /** @var ISharedStorage $sourceStorage */ + /** @var SharedStorage $sourceStorage */ $sourceShare = $sourceStorage->getShare(); foreach ($targetShares as $targetShare) { if ($targetShare->getId() === $sourceShare->getId()) {