diff --git a/apps/dav/lib/Connector/Sabre/ServerFactory.php b/apps/dav/lib/Connector/Sabre/ServerFactory.php index cb0f1e7bfc6a9..9639f59317b5b 100644 --- a/apps/dav/lib/Connector/Sabre/ServerFactory.php +++ b/apps/dav/lib/Connector/Sabre/ServerFactory.php @@ -186,7 +186,8 @@ public function createServer( $server->addPlugin(new SharesPlugin( $tree, $this->userSession, - \OCP\Server::get(\OCP\Share\IManager::class) + \OCP\Server::get(\OCP\Share\IManager::class), + \OCP\Server::get(IRootFolder::class), )); $server->addPlugin(new CommentPropertiesPlugin(\OCP\Server::get(ICommentsManager::class), $this->userSession)); $server->addPlugin(new FilesReportPlugin( diff --git a/apps/dav/lib/Connector/Sabre/SharesPlugin.php b/apps/dav/lib/Connector/Sabre/SharesPlugin.php index bf0205f2565c7..1a9eb88cfe46b 100644 --- a/apps/dav/lib/Connector/Sabre/SharesPlugin.php +++ b/apps/dav/lib/Connector/Sabre/SharesPlugin.php @@ -12,6 +12,7 @@ use OCA\DAV\Connector\Sabre\Exception\Forbidden; use OCA\DAV\Connector\Sabre\Node as DavNode; use OCP\Files\Folder; +use OCP\Files\IRootFolder; use OCP\Files\Node; use OCP\Files\NotFoundException; use OCP\Files\Storage\ISharedStorage; @@ -52,6 +53,7 @@ public function __construct( private Tree $tree, IUserSession $userSession, private IManager $shareManager, + private IRootFolder $rootFolder, ) { $this->userId = $userSession->getUser()->getUID(); } @@ -84,7 +86,7 @@ public function initialize(Server $server) { * @param Node $node * @return IShare[] */ - private function getShare(Node $node): array { + private function getShare(Node $node, bool $includeIncoming = true): array { $result = []; $requestedShareTypes = [ IShare::TYPE_USER, @@ -106,6 +108,10 @@ private function getShare(Node $node): array { -1 ); + if (!$includeIncoming) { + continue; + } + // Also check for shares where the user is the recipient try { $result[] = $this->shareManager->getSharedWith( @@ -122,6 +128,24 @@ private function getShare(Node $node): array { return array_merge(...$result); } + /** + * @return IShare[] + */ + private function getSharesForTarget(Node $node): array { + $shares = $this->getShare($node); + if ($shares !== []) { + return $shares; + } + + // also check the owner side + $userRoot = $this->rootFolder->getUserFolder($this->userId); + while (str_starts_with($node->getPath(), $userRoot->getPath() . '/')) { + $shares = array_merge($shares, $this->getShare($node, false)); + $node = $node->getParent(); + } + return $shares; + } + /** * @param Folder $node * @return IShare[][] @@ -237,8 +261,8 @@ public function validateMoveOrCopy(string $source, string $target): bool { return true; } - $targetShares = $this->getShare($targetNode->getNode()); - if (empty($targetShares)) { + $targetShares = $this->getSharesForTarget($targetNode->getNode()); + if ($targetShares === []) { // Target is not a share so no re-sharing inprogress return true; } @@ -259,7 +283,7 @@ public function validateMoveOrCopy(string $source, string $target): bool { // the user moving the file out of the share to their home storage would give them share permissions and allow moving into the share // // since the 2-step move is allowed, we also allow both steps at once - if ($sourceNode->isDeletable()) { + if ($sourceNode->getInternalPath() !== '' && $sourceNode->isDeletable()) { return true; } } diff --git a/apps/dav/lib/Server.php b/apps/dav/lib/Server.php index fdaa35e4a4d16..c763bb6e58955 100644 --- a/apps/dav/lib/Server.php +++ b/apps/dav/lib/Server.php @@ -343,6 +343,7 @@ public function __construct( $this->server->tree, $userSession, $shareManager, + \OCP\Server::get(IRootFolder::class), )); $this->server->addPlugin(new CommentPropertiesPlugin( \OCP\Server::get(ICommentsManager::class), diff --git a/apps/dav/tests/unit/Connector/Sabre/SharesPluginTest.php b/apps/dav/tests/unit/Connector/Sabre/SharesPluginTest.php index 30b192fe4d910..c90105b3d621f 100644 --- a/apps/dav/tests/unit/Connector/Sabre/SharesPluginTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/SharesPluginTest.php @@ -15,6 +15,7 @@ use OCA\DAV\Connector\Sabre\SharesPlugin; use OCA\DAV\Upload\UploadFile; use OCP\Files\Folder; +use OCP\Files\IRootFolder; use OCP\IUser; use OCP\IUserSession; use OCP\Share\IManager; @@ -28,6 +29,7 @@ class SharesPluginTest extends \Test\TestCase { private \Sabre\DAV\Server $server; private \Sabre\DAV\Tree&MockObject $tree; private \OCP\Share\IManager&MockObject $shareManager; + private IRootFolder&MockObject $rootFolder; private SharesPlugin $plugin; protected function setUp(): void { @@ -35,6 +37,7 @@ protected function setUp(): void { $this->server = new \Sabre\DAV\Server(); $this->tree = $this->createMock(Tree::class); $this->shareManager = $this->createMock(IManager::class); + $this->rootFolder = $this->createMock(IRootFolder::class); $user = $this->createMock(IUser::class); $user->expects($this->once()) ->method('getUID') @@ -47,7 +50,8 @@ protected function setUp(): void { $this->plugin = new SharesPlugin( $this->tree, $userSession, - $this->shareManager + $this->shareManager, + $this->rootFolder, ); $this->plugin->initialize($this->server); } diff --git a/build/integration/sharing_features/sharing-v1-part4.feature b/build/integration/sharing_features/sharing-v1-part4.feature index 5e710d7f28647..4409ebb1116be 100644 --- a/build/integration/sharing_features/sharing-v1-part4.feature +++ b/build/integration/sharing_features/sharing-v1-part4.feature @@ -316,6 +316,117 @@ Scenario: Can copy file between shares if share permissions When User "user1" copies file "/share/test.txt" to "/re-share/movetest.txt" Then the HTTP status code should be "201" +Scenario: Cannot copy files from share without share permission into subfolder of 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 | 7 | + 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" + And user "user1" created a folder "/re-share/subfolder" + 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/subfolder/copytest.txt" + Then the HTTP status code should be "403" + +Scenario: Cannot move files from share without share permission into subfolder of 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 | 7 | + 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" + And user "user1" created a folder "/re-share/subfolder" + 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/subfolder/movetest.txt" + Then the HTTP status code should be "403" + +Scenario: Cannot move folder containing share without share permission into subfolder of 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 | 7 | + 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" + And user "user1" created a folder "/re-share/subfolder" + 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/subfolder/movetest" + Then the HTTP status code should be "403" + +Scenario: Can copy file between shares into subfolder 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" + And user "user1" created a folder "/re-share/subfolder" + 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/subfolder/copytest.txt" + Then the HTTP status code should be "201" + Scenario: Group deletes removes mount without marking Given As an "admin" And user "user0" exists