From 3571207bd956de5dc8aece2ba879f31f3696fef6 Mon Sep 17 00:00:00 2001 From: Bjoern Schiessle Date: Thu, 30 Jun 2016 11:09:20 +0200 Subject: [PATCH 1/6] add some additonal permission checks to the webdav backend --- apps/dav/lib/Connector/Sabre/ObjectTree.php | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/apps/dav/lib/Connector/Sabre/ObjectTree.php b/apps/dav/lib/Connector/Sabre/ObjectTree.php index 9e7d876187..07052e3030 100644 --- a/apps/dav/lib/Connector/Sabre/ObjectTree.php +++ b/apps/dav/lib/Connector/Sabre/ObjectTree.php @@ -71,7 +71,7 @@ class ObjectTree extends \Sabre\DAV\Tree { * is present. * * @param string $path chunk file path to convert - * + * * @return string path to real file */ private function resolveChunkFile($path) { @@ -196,6 +196,15 @@ class ObjectTree extends \Sabre\DAV\Tree { throw new \Sabre\DAV\Exception\ServiceUnavailable('filesystem not setup'); } + $infoDestination = $this->fileView->getFileInfo(dirname($destinationPath)); + $infoSource = $this->fileView->getFileInfo($sourcePath); + $destinationPermission = $infoDestination && $infoDestination->isUpdateable(); + $sourcePermission = $infoSource && $infoSource->isDeletable(); + + if (!$destinationPermission || !$sourcePermission) { + throw new Forbidden(); + } + $targetNodeExists = $this->nodeExists($destinationPath); $sourceNode = $this->getNodeForPath($sourcePath); if ($sourceNode instanceof \Sabre\DAV\ICollection && $targetNodeExists) { @@ -273,6 +282,12 @@ class ObjectTree extends \Sabre\DAV\Tree { throw new \Sabre\DAV\Exception\ServiceUnavailable('filesystem not setup'); } + + $info = $this->fileView->getFileInfo(dirname($destination)); + if ($info && !$info->isUpdateable()) { + throw new Forbidden(); + } + // this will trigger existence check $this->getNodeForPath($source); From b32b296ed770044b347e9216f3f4ad68c55b14f3 Mon Sep 17 00:00:00 2001 From: Lukas Reschke Date: Thu, 30 Jun 2016 12:15:58 +0200 Subject: [PATCH 2/6] Add integration tests --- .../integration/features/bootstrap/WebDav.php | 18 ++++++++++++ .../features/webdav-related.feature | 29 +++++++++++++++++++ 2 files changed, 47 insertions(+) diff --git a/build/integration/features/bootstrap/WebDav.php b/build/integration/features/bootstrap/WebDav.php index 0abb866773..23f80c2fa7 100644 --- a/build/integration/features/bootstrap/WebDav.php +++ b/build/integration/features/bootstrap/WebDav.php @@ -42,6 +42,7 @@ trait WebDav { $request->setBody($body); } + return $client->send($request); } @@ -70,6 +71,23 @@ trait WebDav { $this->response = $this->makeDavRequest($user, "MOVE", $fileSource, $headers); } + /** + * @When /^User "([^"]*)" copies file "([^"]*)" to "([^"]*)"$/ + * @param string $user + * @param string $fileSource + * @param string $fileDestination + */ + public function userCopiesFileTo($user, $fileSource, $fileDestination) { + $fullUrl = substr($this->baseUrl, 0, -4) . $this->davPath; + $headers['Destination'] = $fullUrl . $fileDestination; + try { + $this->response = $this->makeDavRequest($user, 'COPY', $fileSource, $headers); + } catch (\GuzzleHttp\Exception\ClientException $e) { + // 4xx and 5xx responses cause an exception + $this->response = $e->getResponse(); + } + } + /** * @When /^Downloading file "([^"]*)" with range "([^"]*)"$/ * @param string $fileSource diff --git a/build/integration/features/webdav-related.feature b/build/integration/features/webdav-related.feature index 06df280ea6..a135f077f7 100644 --- a/build/integration/features/webdav-related.feature +++ b/build/integration/features/webdav-related.feature @@ -257,3 +257,32 @@ Feature: webdav-related When Downloading file "/welcome.txt" as "userToBeDisabled" Then the HTTP status code should be "503" + Scenario: Copying files into a folder with edit permissions + Given using dav path "remote.php/webdav" + And user "user0" exists + And user "user1" exists + And As an "user1" + And user "user1" created a folder "/testcopypermissionsAllowed" + And as "user1" creating a share with + | path | testcopypermissionsAllowed | + | shareType | 0 | + | permissions | 31 | + | shareWith | user0 | + And User "user0" uploads file with content "copytest" to "/copytest.txt" + When User "user0" copies file "/copytest.txt" to "/testcopypermissionsAllowed/copytest.txt" + Then the HTTP status code should be "201" + + Scenario: Copying files into a folder without edit permissions + Given using dav path "remote.php/webdav" + And user "user0" exists + And user "user1" exists + And As an "user1" + And user "user1" created a folder "/testcopypermissionsNotAllowed" + And as "user1" creating a share with + | path | testcopypermissionsNotAllowed | + | shareType | 0 | + | permissions | 1 | + | shareWith | user0 | + And User "user0" uploads file with content "copytest" to "/copytest.txt" + When User "user0" copies file "/copytest.txt" to "/testcopypermissionsNotAllowed/copytest.txt" + Then the HTTP status code should be "403" From 1e7f0f73410cf3b8a7fd52cade27de92bc5994c3 Mon Sep 17 00:00:00 2001 From: Lukas Reschke Date: Thu, 30 Jun 2016 13:17:53 +0200 Subject: [PATCH 3/6] Add required $message parameter --- apps/dav/lib/Connector/Sabre/ObjectTree.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/apps/dav/lib/Connector/Sabre/ObjectTree.php b/apps/dav/lib/Connector/Sabre/ObjectTree.php index 07052e3030..100e5ec56a 100644 --- a/apps/dav/lib/Connector/Sabre/ObjectTree.php +++ b/apps/dav/lib/Connector/Sabre/ObjectTree.php @@ -202,7 +202,7 @@ class ObjectTree extends \Sabre\DAV\Tree { $sourcePermission = $infoSource && $infoSource->isDeletable(); if (!$destinationPermission || !$sourcePermission) { - throw new Forbidden(); + throw new Forbidden('No permissions to move object.'); } $targetNodeExists = $this->nodeExists($destinationPath); @@ -285,7 +285,7 @@ class ObjectTree extends \Sabre\DAV\Tree { $info = $this->fileView->getFileInfo(dirname($destination)); if ($info && !$info->isUpdateable()) { - throw new Forbidden(); + throw new Forbidden('No permissions to move object.'); } // this will trigger existence check From c771368c4e3e8fdac4ef95758f308f85d5a29edf Mon Sep 17 00:00:00 2001 From: Lukas Reschke Date: Thu, 30 Jun 2016 13:19:50 +0200 Subject: [PATCH 4/6] Add proper throws PHP docs --- apps/dav/lib/Connector/Sabre/ObjectTree.php | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/apps/dav/lib/Connector/Sabre/ObjectTree.php b/apps/dav/lib/Connector/Sabre/ObjectTree.php index 100e5ec56a..8f5f7be250 100644 --- a/apps/dav/lib/Connector/Sabre/ObjectTree.php +++ b/apps/dav/lib/Connector/Sabre/ObjectTree.php @@ -186,9 +186,13 @@ class ObjectTree extends \Sabre\DAV\Tree { * * @param string $sourcePath The path to the file which should be moved * @param string $destinationPath The full destination path, so not just the destination parent node - * @throws \Sabre\DAV\Exception\BadRequest - * @throws \Sabre\DAV\Exception\ServiceUnavailable + * @throws FileLocked + * @throws Forbidden + * @throws InvalidPath * @throws \Sabre\DAV\Exception\Forbidden + * @throws \Sabre\DAV\Exception\Locked + * @throws \Sabre\DAV\Exception\NotFound + * @throws \Sabre\DAV\Exception\ServiceUnavailable * @return int */ public function move($sourcePath, $destinationPath) { @@ -274,6 +278,13 @@ class ObjectTree extends \Sabre\DAV\Tree { * * @param string $source * @param string $destination + * @throws FileLocked + * @throws Forbidden + * @throws InvalidPath + * @throws \Exception + * @throws \Sabre\DAV\Exception\Forbidden + * @throws \Sabre\DAV\Exception\Locked + * @throws \Sabre\DAV\Exception\NotFound * @throws \Sabre\DAV\Exception\ServiceUnavailable * @return void */ From 149218ead9c43d79e45ba19ea59cd4966a1e046b Mon Sep 17 00:00:00 2001 From: Lukas Reschke Date: Thu, 30 Jun 2016 13:46:08 +0200 Subject: [PATCH 5/6] Fix tests --- .../unit/Connector/Sabre/ObjectTreeTest.php | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/apps/dav/tests/unit/Connector/Sabre/ObjectTreeTest.php b/apps/dav/tests/unit/Connector/Sabre/ObjectTreeTest.php index 4a5e43376c..96d4357660 100644 --- a/apps/dav/tests/unit/Connector/Sabre/ObjectTreeTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/ObjectTreeTest.php @@ -35,6 +35,7 @@ class TestDoubleFileView extends \OC\Files\View { $this->updatables = $updatables; $this->deletables = $deletables; $this->canRename = $canRename; + $this->lockingProvider = \OC::$server->getLockingProvider(); } public function isUpdatable($path) { @@ -56,6 +57,11 @@ class TestDoubleFileView extends \OC\Files\View { public function getRelativePath($path) { return $path; } + + public function getFileInfo($path, $includeMountPoints = true) { + $objectTreeTest = new ObjectTreeTest(); + return $objectTreeTest->getFileInfoMock(); + } } /** @@ -67,6 +73,20 @@ class TestDoubleFileView extends \OC\Files\View { */ class ObjectTreeTest extends \Test\TestCase { + public function getFileInfoMock() { + $mock = $this->getMock('\OCP\Files\FileInfo'); + $mock + ->expects($this->any()) + ->method('isDeletable') + ->willReturn(true); + $mock + ->expects($this->any()) + ->method('isUpdateable') + ->willReturn(true); + + return $mock; + } + /** * @dataProvider moveFailedProvider * @expectedException \Sabre\DAV\Exception\Forbidden From 26e14529be942e3cc3c2bb2b388b155073daecb1 Mon Sep 17 00:00:00 2001 From: Bjoern Schiessle Date: Thu, 30 Jun 2016 13:50:31 +0200 Subject: [PATCH 6/6] fix error message --- apps/dav/lib/Connector/Sabre/ObjectTree.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/dav/lib/Connector/Sabre/ObjectTree.php b/apps/dav/lib/Connector/Sabre/ObjectTree.php index 8f5f7be250..d8c1d71e7f 100644 --- a/apps/dav/lib/Connector/Sabre/ObjectTree.php +++ b/apps/dav/lib/Connector/Sabre/ObjectTree.php @@ -296,7 +296,7 @@ class ObjectTree extends \Sabre\DAV\Tree { $info = $this->fileView->getFileInfo(dirname($destination)); if ($info && !$info->isUpdateable()) { - throw new Forbidden('No permissions to move object.'); + throw new Forbidden('No permissions to copy object.'); } // this will trigger existence check