From 9ab17c95c0746cced8a4f36bec58dc98ee229ef3 Mon Sep 17 00:00:00 2001 From: Sergio Bertolin Date: Mon, 27 Mar 2017 10:27:23 +0000 Subject: [PATCH 01/16] Added test about checking file id after a move --- .../integration/features/bootstrap/WebDav.php | 35 +++++++++++++++++-- .../features/webdav-related.feature | 12 +++++++ 2 files changed, 45 insertions(+), 2 deletions(-) diff --git a/build/integration/features/bootstrap/WebDav.php b/build/integration/features/bootstrap/WebDav.php index 3a018a2d0faca..9242b80ac13fd 100644 --- a/build/integration/features/bootstrap/WebDav.php +++ b/build/integration/features/bootstrap/WebDav.php @@ -45,8 +45,10 @@ trait WebDav { private $usingOldDavPath = true; /** @var ResponseInterface */ private $response; - /** @var map with user as key and another map as value, which has path as key and etag as value */ - private $storedETAG = NULL; + /** @var array map with user as key and another map as value, which has path as key and etag as value */ + private $storedETAG = null; + /** @var int */ + private $storedFileID = null; /** * @Given /^using dav path "([^"]*)"$/ @@ -749,4 +751,33 @@ public function userDeletesEverythingInFolder($user, $folder) { } + /** + * @param string $user + * @param string $path + * @return int + */ + private function getFileIdForPath($user, $path) { + $propertiesTable = new \Behat\Gherkin\Node\TableNode([["{http://owncloud.org/ns}fileid"]]); + $this->asGetsPropertiesOfFolderWith($user, 'file', $path, $propertiesTable); + return (int) $this->response['{http://owncloud.org/ns}fileid']; + } + + /** + * @Given /^User "([^"]*)" stores id of file "([^"]*)"$/ + * @param string $user + * @param string $path + */ + public function userStoresFileIdForPath($user, $path) { + $this->storedFileID = $this->getFileIdForPath($user, $path); + } + + /** + * @Given /^User "([^"]*)" checks id of file "([^"]*)"$/ + * @param string $user + * @param string $path + */ + public function userChecksFileIdForPath($user, $path) { + $currentFileID = $this->getFileIdForPath($user, $path); + PHPUnit_Framework_Assert::assertEquals($currentFileID, $this->storedFileID); + } } diff --git a/build/integration/features/webdav-related.feature b/build/integration/features/webdav-related.feature index 6aee59036d3b0..457d92d52a6d4 100644 --- a/build/integration/features/webdav-related.feature +++ b/build/integration/features/webdav-related.feature @@ -517,3 +517,15 @@ Feature: webdav-related | /textfile2.txt | | /textfile3.txt | | /textfile4.txt | + + Scenario: Checking file id after a move using new endpoint + Given using new dav path + And user "user0" exists + And user "user0" creates a new chunking upload with id "chunking-42" + And user "user0" uploads new chunk file "1" with "AAAAA" to id "chunking-42" + And user "user0" uploads new chunk file "2" with "BBBBB" to id "chunking-42" + And user "user0" uploads new chunk file "3" with "CCCCC" to id "chunking-42" + And user "user0" moves new chunk file with id "chunking-42" to "/myChunkedFile.txt" + And User "user0" stores id of file "/myChunkedFile.txt" + When User "user0" moves file "/myChunkedFile.txt" to "/FOLDER/myChunkedFile.txt" + Then User "user0" checks id of file "/FOLDER/myChunkedFile.txt" From c30feafaa2f2fb86469f5c38a6e20ed3f3042873 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Mon, 27 Mar 2017 18:50:02 +0200 Subject: [PATCH 02/16] Added case when final chunk move must not change file id --- build/integration/features/webdav-related.feature | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/build/integration/features/webdav-related.feature b/build/integration/features/webdav-related.feature index 457d92d52a6d4..231f49f439863 100644 --- a/build/integration/features/webdav-related.feature +++ b/build/integration/features/webdav-related.feature @@ -529,3 +529,16 @@ Feature: webdav-related And User "user0" stores id of file "/myChunkedFile.txt" When User "user0" moves file "/myChunkedFile.txt" to "/FOLDER/myChunkedFile.txt" Then User "user0" checks id of file "/FOLDER/myChunkedFile.txt" + + Scenario: Checking file id after a move with overwrite using new endpoint + Given using new dav path + And user "user0" exists + And User "user0" copies file "/textfile0.txt" to "/existingFile.txt" + And User "user0" stores id of file "/existingFile.txt" + And user "user0" creates a new chunking upload with id "chunking-42" + And user "user0" uploads new chunk file "1" with "AAAAA" to id "chunking-42" + And user "user0" uploads new chunk file "2" with "BBBBB" to id "chunking-42" + And user "user0" uploads new chunk file "3" with "CCCCC" to id "chunking-42" + When user "user0" moves new chunk file with id "chunking-42" to "/existingFile.txt" + Then User "user0" checks id of file "/existingFile.txt" + From 9bff66e68d9b2a5712eae406b54d103b51a8c7e9 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Mon, 27 Mar 2017 18:53:06 +0200 Subject: [PATCH 03/16] Simplified new endpoint move test --- build/integration/features/webdav-related.feature | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/build/integration/features/webdav-related.feature b/build/integration/features/webdav-related.feature index 231f49f439863..14033a69a72bf 100644 --- a/build/integration/features/webdav-related.feature +++ b/build/integration/features/webdav-related.feature @@ -521,16 +521,11 @@ Feature: webdav-related Scenario: Checking file id after a move using new endpoint Given using new dav path And user "user0" exists - And user "user0" creates a new chunking upload with id "chunking-42" - And user "user0" uploads new chunk file "1" with "AAAAA" to id "chunking-42" - And user "user0" uploads new chunk file "2" with "BBBBB" to id "chunking-42" - And user "user0" uploads new chunk file "3" with "CCCCC" to id "chunking-42" - And user "user0" moves new chunk file with id "chunking-42" to "/myChunkedFile.txt" - And User "user0" stores id of file "/myChunkedFile.txt" - When User "user0" moves file "/myChunkedFile.txt" to "/FOLDER/myChunkedFile.txt" - Then User "user0" checks id of file "/FOLDER/myChunkedFile.txt" + And User "user0" stores id of file "/textfile0.txt" + When User "user0" moves file "/textfile0.txt" to "/FOLDER/textfile0.txt" + Then User "user0" checks id of file "/FOLDER/textfile0.txt" - Scenario: Checking file id after a move with overwrite using new endpoint + Scenario: Checking file id after a move overwrite using new chunking endpoint Given using new dav path And user "user0" exists And User "user0" copies file "/textfile0.txt" to "/existingFile.txt" From a761d4cce725435036c980667109ddb25126aa31 Mon Sep 17 00:00:00 2001 From: Sergio Bertolin Date: Wed, 29 Mar 2017 09:21:13 +0000 Subject: [PATCH 04/16] Added test cases from core 16825 --- .../features/webdav-related.feature | 41 +++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/build/integration/features/webdav-related.feature b/build/integration/features/webdav-related.feature index 14033a69a72bf..d0eb3fb4bf4f2 100644 --- a/build/integration/features/webdav-related.feature +++ b/build/integration/features/webdav-related.feature @@ -537,3 +537,44 @@ Feature: webdav-related When user "user0" moves new chunk file with id "chunking-42" to "/existingFile.txt" Then User "user0" checks id of file "/existingFile.txt" + Scenario: Renaming a folder to a backslash encoded should return an error using old endpoint + Given using old dav path + And user "user0" exists + And user "user0" created a folder "/testshare" + When User "user0" moves folder "/testshare" to "/%2F" + Then the HTTP status code should be "403" + + Scenario: Renaming a folder beginning with a backslash encoded should return an error using old endpoint + Given using old dav path + And user "user0" exists + And user "user0" created a folder "/testshare" + When User "user0" moves folder "/testshare" to "/%2Ftestshare" + Then the HTTP status code should be "403" + + Scenario: Renaming a folder including a backslash encoded should return an error using old endpoint + Given using old dav path + And user "user0" exists + And user "user0" created a folder "/testshare" + When User "user0" moves folder "/testshare" to "/hola%2Fhola" + Then the HTTP status code should be "403" + + Scenario: Renaming a folder to a backslash encoded should return an error using new endpoint + Given using new dav path + And user "user0" exists + And user "user0" created a folder "/testshare" + When User "user0" moves folder "/testshare" to "/%2F" + Then the HTTP status code should be "403" + + Scenario: Renaming a folder beginning with a backslash encoded should return an error using new endpoint + Given using new dav path + And user "user0" exists + And user "user0" created a folder "/testshare" + When User "user0" moves folder "/testshare" to "/%2Ftestshare" + Then the HTTP status code should be "403" + + Scenario: Renaming a folder including a backslash encoded should return an error using new endpoint + Given using new dav path + And user "user0" exists + And user "user0" created a folder "/testshare" + When User "user0" moves folder "/testshare" to "/hola%2Fhola" + Then the HTTP status code should be "403" From 0a9f7730d0daeed565af28ad2e1528606bab6ba7 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Wed, 29 Mar 2017 11:04:27 +0200 Subject: [PATCH 05/16] Ported ObjectTree::move to IMoveTarget in new DAV endpoint --- apps/dav/lib/Connector/Sabre/Directory.php | 114 ++++++++++++++++++++- 1 file changed, 110 insertions(+), 4 deletions(-) diff --git a/apps/dav/lib/Connector/Sabre/Directory.php b/apps/dav/lib/Connector/Sabre/Directory.php index 25cca40a88980..a26420344d040 100644 --- a/apps/dav/lib/Connector/Sabre/Directory.php +++ b/apps/dav/lib/Connector/Sabre/Directory.php @@ -34,12 +34,18 @@ use OCA\DAV\Connector\Sabre\Exception\InvalidPath; use OCA\DAV\Connector\Sabre\Exception\FileLocked; use OCP\Files\ForbiddenException; +use OCP\Files\InvalidPathException; +use OCP\Files\StorageNotAvailableException; use OCP\Lock\ILockingProvider; use OCP\Lock\LockedException; use Sabre\DAV\Exception\Locked; +use Sabre\DAV\Exception\ServiceUnavailable; +use Sabre\DAV\INode; +use Sabre\DAV\Exception\BadRequest; +use OC\Files\Mount\MoveableMount; class Directory extends \OCA\DAV\Connector\Sabre\Node - implements \Sabre\DAV\ICollection, \Sabre\DAV\IQuota { + implements \Sabre\DAV\ICollection, \Sabre\DAV\IQuota, \Sabre\DAV\IMoveTarget { /** * Cached directory content @@ -137,7 +143,7 @@ public function createFile($name, $data = null) { return $node->put($data); } catch (\OCP\Files\StorageNotAvailableException $e) { throw new \Sabre\DAV\Exception\ServiceUnavailable($e->getMessage()); - } catch (\OCP\Files\InvalidPathException $ex) { + } catch (InvalidPathException $ex) { throw new InvalidPath($ex->getMessage()); } catch (ForbiddenException $ex) { throw new Forbidden($ex->getMessage(), $ex->getRetry()); @@ -168,7 +174,7 @@ public function createDirectory($name) { } } catch (\OCP\Files\StorageNotAvailableException $e) { throw new \Sabre\DAV\Exception\ServiceUnavailable($e->getMessage()); - } catch (\OCP\Files\InvalidPathException $ex) { + } catch (InvalidPathException $ex) { throw new InvalidPath($ex->getMessage()); } catch (ForbiddenException $ex) { throw new Forbidden($ex->getMessage(), $ex->getRetry()); @@ -195,7 +201,7 @@ public function getChild($name, $info = null) { $info = $this->fileView->getFileInfo($path); } catch (\OCP\Files\StorageNotAvailableException $e) { throw new \Sabre\DAV\Exception\ServiceUnavailable($e->getMessage()); - } catch (\OCP\Files\InvalidPathException $ex) { + } catch (InvalidPathException $ex) { throw new InvalidPath($ex->getMessage()); } catch (ForbiddenException $e) { throw new \Sabre\DAV\Exception\Forbidden(); @@ -311,4 +317,104 @@ public function getQuotaInfo() { } } + /** + * Moves a node into this collection. + * + * It is up to the implementors to: + * 1. Create the new resource. + * 2. Remove the old resource. + * 3. Transfer any properties or other data. + * + * Generally you should make very sure that your collection can easily move + * the move. + * + * If you don't, just return false, which will trigger sabre/dav to handle + * the move itself. If you return true from this function, the assumption + * is that the move was successful. + * + * @param string $targetName New local file/collection name. + * @param string $fullSourcePath Full path to source node + * @param INode $sourceNode Source node itself + * @return bool + * @throws BadRequest + * @throws ServiceUnavailable + * @throws Forbidden + * @throws FileLocked + * @throws \Sabre\DAV\Exception\Forbidden + */ + public function moveInto($targetName, $fullSourcePath, INode $sourceNode) { + if (!$sourceNode instanceof Node) { + throw new BadRequest('Incompatible node types'); + } + + if (!$this->fileView) { + throw new ServiceUnavailable('filesystem not setup'); + } + + $destinationPath = $this->getPath() . '/' . $targetName; + + + $targetNodeExists = $this->childExists($targetName); + + // at getNodeForPath we also check the path for isForbiddenFileOrDir + // with that we have covered both source and destination + if ($sourceNode instanceof Directory && $targetNodeExists) { + throw new \Sabre\DAV\Exception\Forbidden('Could not copy directory ' . $sourceNode->getName() . ', target exists'); + } + + list($sourceDir,) = \Sabre\HTTP\URLUtil::splitPath($sourceNode->getPath()); + $destinationDir = $this->getPath(); + + $sourcePath = $sourceNode->getPath(); + + $isMovableMount = false; + $sourceMount = \OC::$server->getMountManager()->find($this->fileView->getAbsolutePath($sourcePath)); + $internalPath = $sourceMount->getInternalPath($this->fileView->getAbsolutePath($sourcePath)); + if ($sourceMount instanceof MoveableMount && $internalPath === '') { + $isMovableMount = true; + } + + try { + $sameFolder = ($sourceDir === $destinationDir); + // if we're overwriting or same folder + if ($targetNodeExists || $sameFolder) { + // note that renaming a share mount point is always allowed + if (!$this->fileView->isUpdatable($destinationDir) && !$isMovableMount) { + throw new \Sabre\DAV\Exception\Forbidden(); + } + } else { + if (!$this->fileView->isCreatable($destinationDir)) { + throw new \Sabre\DAV\Exception\Forbidden(); + } + } + + if (!$sameFolder) { + // moving to a different folder, source will be gone, like a deletion + // note that moving a share mount point is always allowed + if (!$this->fileView->isDeletable($sourcePath) && !$isMovableMount) { + throw new \Sabre\DAV\Exception\Forbidden(); + } + } + + $fileName = basename($destinationPath); + try { + $this->fileView->verifyPath($destinationDir, $fileName); + } catch (InvalidPathException $ex) { + throw new InvalidPath($ex->getMessage()); + } + + $renameOkay = $this->fileView->rename($sourcePath, $destinationPath); + if (!$renameOkay) { + throw new \Sabre\DAV\Exception\Forbidden(''); + } + } catch (StorageNotAvailableException $e) { + throw new ServiceUnavailable($e->getMessage()); + } catch (ForbiddenException $ex) { + throw new Forbidden($ex->getMessage(), $ex->getRetry()); + } catch (LockedException $e) { + throw new FileLocked($e->getMessage(), $e->getCode(), $e); + } + + return true; + } } From 82b967d3f9f661b784f5ba97d5a69d0b40a1c7be Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Wed, 29 Mar 2017 11:16:41 +0200 Subject: [PATCH 06/16] Remove ObjectTree::move and let is use the IMoveTarget approach instead This removes the duplicated code --- apps/dav/lib/Connector/Sabre/ObjectTree.php | 98 --------------------- 1 file changed, 98 deletions(-) diff --git a/apps/dav/lib/Connector/Sabre/ObjectTree.php b/apps/dav/lib/Connector/Sabre/ObjectTree.php index 554a7ad86ca8a..acc6dcc3be3cf 100644 --- a/apps/dav/lib/Connector/Sabre/ObjectTree.php +++ b/apps/dav/lib/Connector/Sabre/ObjectTree.php @@ -184,104 +184,6 @@ public function getNodeForPath($path) { } - /** - * Moves a file from one location to another - * - * @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 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) { - if (!$this->fileView) { - throw new \Sabre\DAV\Exception\ServiceUnavailable('filesystem not setup'); - } - - $infoDestination = $this->fileView->getFileInfo(dirname($destinationPath)); - if (dirname($destinationPath) === dirname($sourcePath)) { - $sourcePermission = $infoDestination && $infoDestination->isUpdateable(); - $destinationPermission = $sourcePermission; - } else { - $infoSource = $this->fileView->getFileInfo($sourcePath); - if ($this->fileView->file_exists($destinationPath)) { - $destinationPermission = $infoDestination && $infoDestination->isUpdateable(); - } else { - $destinationPermission = $infoDestination && $infoDestination->isCreatable(); - } - $sourcePermission = $infoSource && $infoSource->isDeletable(); - } - - if (!$destinationPermission || !$sourcePermission) { - throw new Forbidden('No permissions to move object.'); - } - - $targetNodeExists = $this->nodeExists($destinationPath); - $sourceNode = $this->getNodeForPath($sourcePath); - if ($sourceNode instanceof \Sabre\DAV\ICollection && $targetNodeExists) { - throw new \Sabre\DAV\Exception\Forbidden('Could not copy directory ' . $sourceNode->getName() . ', target exists'); - } - list($sourceDir,) = \Sabre\HTTP\URLUtil::splitPath($sourcePath); - list($destinationDir,) = \Sabre\HTTP\URLUtil::splitPath($destinationPath); - - $isMovableMount = false; - $sourceMount = $this->mountManager->find($this->fileView->getAbsolutePath($sourcePath)); - $internalPath = $sourceMount->getInternalPath($this->fileView->getAbsolutePath($sourcePath)); - if ($sourceMount instanceof MoveableMount && $internalPath === '') { - $isMovableMount = true; - } - - try { - $sameFolder = ($sourceDir === $destinationDir); - // if we're overwriting or same folder - if ($targetNodeExists || $sameFolder) { - // note that renaming a share mount point is always allowed - if (!$this->fileView->isUpdatable($destinationDir) && !$isMovableMount) { - throw new \Sabre\DAV\Exception\Forbidden(); - } - } else { - if (!$this->fileView->isCreatable($destinationDir)) { - throw new \Sabre\DAV\Exception\Forbidden(); - } - } - - if (!$sameFolder) { - // moving to a different folder, source will be gone, like a deletion - // note that moving a share mount point is always allowed - if (!$this->fileView->isDeletable($sourcePath) && !$isMovableMount) { - throw new \Sabre\DAV\Exception\Forbidden(); - } - } - - $fileName = basename($destinationPath); - try { - $this->fileView->verifyPath($destinationDir, $fileName); - } catch (\OCP\Files\InvalidPathException $ex) { - throw new InvalidPath($ex->getMessage()); - } - - $renameOkay = $this->fileView->rename($sourcePath, $destinationPath); - if (!$renameOkay) { - throw new \Sabre\DAV\Exception\Forbidden(''); - } - } catch (StorageNotAvailableException $e) { - throw new \Sabre\DAV\Exception\ServiceUnavailable($e->getMessage()); - } catch (ForbiddenException $ex) { - throw new Forbidden($ex->getMessage(), $ex->getRetry()); - } catch (LockedException $e) { - throw new FileLocked($e->getMessage(), $e->getCode(), $e); - } - - $this->markDirty($sourceDir); - $this->markDirty($destinationDir); - - } - /** * Copies a file or directory. * From 642b4331a6c0aec3a6a7d0350f3582df7caafc6b Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Wed, 29 Mar 2017 12:15:42 +0200 Subject: [PATCH 07/16] Moved unit tests from ObjectTree::move to Directory --- .../unit/Connector/Sabre/DirectoryTest.php | 170 +++++++++++++++++- .../unit/Connector/Sabre/ObjectTreeTest.php | 94 +++++++++- lib/private/Files/View.php | 2 +- 3 files changed, 256 insertions(+), 10 deletions(-) diff --git a/apps/dav/tests/unit/Connector/Sabre/DirectoryTest.php b/apps/dav/tests/unit/Connector/Sabre/DirectoryTest.php index 18f91bbd8c94a..1eb3741abf237 100644 --- a/apps/dav/tests/unit/Connector/Sabre/DirectoryTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/DirectoryTest.php @@ -27,6 +27,42 @@ namespace OCA\DAV\Tests\Unit\Connector\Sabre; use OCP\Files\ForbiddenException; +use OC\Files\FileInfo; +use OCA\DAV\Connector\Sabre\Directory; + +class TestDoubleFileView extends \OC\Files\View { + + private $updatables; + private $deletables; + private $canRename; + + public function __construct($updatables, $deletables, $canRename = true) { + $this->updatables = $updatables; + $this->deletables = $deletables; + $this->canRename = $canRename; + } + + public function isUpdatable($path) { + return $this->updatables[$path]; + } + + public function isCreatable($path) { + return $this->updatables[$path]; + } + + public function isDeletable($path) { + return $this->deletables[$path]; + } + + public function rename($path1, $path2) { + return $this->canRename; + } + + public function getRelativePath($path) { + return $path; + } +} + /** * @group DB @@ -58,7 +94,7 @@ private function getDir($path = '/') { ->method('getPath') ->will($this->returnValue($path)); - return new \OCA\DAV\Connector\Sabre\Directory($this->view, $this->info); + return new Directory($this->view, $this->info); } /** @@ -172,7 +208,7 @@ public function testGetChildren() { ->method('getRelativePath') ->will($this->returnValue('')); - $dir = new \OCA\DAV\Connector\Sabre\Directory($this->view, $this->info); + $dir = new Directory($this->view, $this->info); $nodes = $dir->getChildren(); $this->assertEquals(2, count($nodes)); @@ -182,6 +218,30 @@ public function testGetChildren() { $dir->getChildren(); } + /** + * @expectedException \Sabre\DAV\Exception\Forbidden + */ + public function testGetChildrenNoPermission() { + $this->info->expects($this->any()) + ->method('isReadable') + ->will($this->returnValue(false)); + + $dir = new Directory($this->view, $this->info); + $dir->getChildren(); + } + + /** + * @expectedException \Sabre\DAV\Exception\NotFound + */ + public function testGetChildNoPermission() { + $this->info->expects($this->any()) + ->method('isReadable') + ->will($this->returnValue(false)); + + $dir = new Directory($this->view, $this->info); + $dir->getChild('test'); + } + /** * @expectedException \Sabre\DAV\Exception\ServiceUnavailable */ @@ -190,7 +250,7 @@ public function testGetChildThrowStorageNotAvailableException() { ->method('getFileInfo') ->willThrowException(new \OCP\Files\StorageNotAvailableException()); - $dir = new \OCA\DAV\Connector\Sabre\Directory($this->view, $this->info); + $dir = new Directory($this->view, $this->info); $dir->getChild('.'); } @@ -204,7 +264,7 @@ public function testGetChildThrowInvalidPath() { $this->view->expects($this->never()) ->method('getFileInfo'); - $dir = new \OCA\DAV\Connector\Sabre\Directory($this->view, $this->info); + $dir = new Directory($this->view, $this->info); $dir->getChild('.'); } @@ -235,7 +295,7 @@ public function testGetQuotaInfoUnlimited() { ->method('getStorage') ->will($this->returnValue($storage)); - $dir = new \OCA\DAV\Connector\Sabre\Directory($this->view, $this->info); + $dir = new Directory($this->view, $this->info); $this->assertEquals([200, -3], $dir->getQuotaInfo()); //200 used, unlimited } @@ -267,7 +327,105 @@ public function testGetQuotaInfoSpecific() { ->method('getStorage') ->will($this->returnValue($storage)); - $dir = new \OCA\DAV\Connector\Sabre\Directory($this->view, $this->info); + $dir = new Directory($this->view, $this->info); $this->assertEquals([200, 800], $dir->getQuotaInfo()); //200 used, 800 free } + + /** + * @dataProvider moveFailedProvider + * @expectedException \Sabre\DAV\Exception\Forbidden + */ + public function testMoveFailed($source, $destination, $updatables, $deletables) { + $this->moveTest($source, $destination, $updatables, $deletables); + } + + /** + * @dataProvider moveSuccessProvider + */ + public function testMoveSuccess($source, $destination, $updatables, $deletables) { + $this->moveTest($source, $destination, $updatables, $deletables); + $this->assertTrue(true); + } + + /** + * @dataProvider moveFailedInvalidCharsProvider + * @expectedException \OCA\DAV\Connector\Sabre\Exception\InvalidPath + */ + public function testMoveFailedInvalidChars($source, $destination, $updatables, $deletables) { + $this->moveTest($source, $destination, $updatables, $deletables); + } + + public function moveFailedInvalidCharsProvider() { + return [ + ['a/b', 'a/*', ['a' => true, 'a/b' => true, 'a/c*' => false], []], + ]; + } + + public function moveFailedProvider() { + return [ + ['a/b', 'a/c', ['a' => false, 'a/b' => false, 'a/c' => false], []], + ['a/b', 'b/b', ['a' => false, 'a/b' => false, 'b' => false, 'b/b' => false], []], + ['a/b', 'b/b', ['a' => false, 'a/b' => true, 'b' => false, 'b/b' => false], []], + ['a/b', 'b/b', ['a' => true, 'a/b' => true, 'b' => false, 'b/b' => false], []], + ['a/b', 'b/b', ['a' => true, 'a/b' => true, 'b' => true, 'b/b' => false], ['a/b' => false]], + ['a/b', 'a/c', ['a' => false, 'a/b' => true, 'a/c' => false], []], + ]; + } + + public function moveSuccessProvider() { + return [ + ['a/b', 'b/b', ['a' => true, 'a/b' => true, 'b' => true, 'b/b' => false], ['a/b' => true]], + // older files with special chars can still be renamed to valid names + ['a/b*', 'b/b', ['a' => true, 'a/b*' => true, 'b' => true, 'b/b' => false], ['a/b*' => true]], + ]; + } + + /** + * @param $source + * @param $destination + * @param $updatables + */ + private function moveTest($source, $destination, $updatables, $deletables) { + $view = new TestDoubleFileView($updatables, $deletables); + + $sourceInfo = new FileInfo($source, null, null, [], null); + $targetInfo = new FileInfo(dirname($destination), null, null, [], null); + + $sourceNode = new Directory($view, $sourceInfo); + $targetNode = $this->getMockBuilder(Directory::class) + ->setMethods(['childExists']) + ->setConstructorArgs([$view, $targetInfo]) + ->getMock(); + $targetNode->expects($this->any())->method('childExists') + ->with(basename($destination)) + ->willReturn(false); + $this->assertTrue($targetNode->moveInto(basename($destination), $source, $sourceNode)); + } + + /** + * @expectedException \Sabre\DAV\Exception\Forbidden + * @expectedExceptionMessage Could not copy directory b, target exists + */ + public function testFailingMove() { + $source = 'a/b'; + $destination = 'c/b'; + $updatables = ['a' => true, 'a/b' => true, 'b' => true, 'c/b' => false]; + $deletables = ['a/b' => true]; + + $view = new TestDoubleFileView($updatables, $deletables); + + $sourceInfo = new FileInfo($source, null, null, [], null); + $targetInfo = new FileInfo(dirname($destination), null, null, [], null); + + $sourceNode = new Directory($view, $sourceInfo); + $targetNode = $this->getMockBuilder(Directory::class) + ->setMethods(['childExists']) + ->setConstructorArgs([$view, $targetInfo]) + ->getMock(); + $targetNode->expects($this->once())->method('childExists') + ->with(basename($destination)) + ->willReturn(true); + + $targetNode->moveInto(basename($destination), $source, $sourceNode); + } } diff --git a/apps/dav/tests/unit/Connector/Sabre/ObjectTreeTest.php b/apps/dav/tests/unit/Connector/Sabre/ObjectTreeTest.php index 17cb598bf6e81..098d4e2702324 100644 --- a/apps/dav/tests/unit/Connector/Sabre/ObjectTreeTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/ObjectTreeTest.php @@ -30,7 +30,11 @@ use OC\Files\FileInfo; +use OC\Files\Filesystem; use OC\Files\Storage\Temporary; +use OC\Files\View; +use OCA\DAV\Connector\Sabre\Directory; +use OCA\DAV\Connector\Sabre\ObjectTree; class TestDoubleFileView extends \OC\Files\View { @@ -125,13 +129,13 @@ public function testMoveFailedInvalidChars($source, $destination, $updatables, $ $this->moveTest($source, $destination, $updatables, $updatables, $deletables); } - function moveFailedInvalidCharsProvider() { + public function moveFailedInvalidCharsProvider() { return array( array('a/b', 'a/*', array('a' => true, 'a/b' => true, 'a/c*' => false), array()), ); } - function moveFailedProvider() { + public function moveFailedProvider() { return array( array('a/b', 'a/c', array('a' => false, 'a/b' => false, 'a/c' => false), array()), array('a/b', 'b/b', array('a' => false, 'a/b' => false, 'b' => false, 'b/b' => false), array()), @@ -142,7 +146,7 @@ function moveFailedProvider() { ); } - function moveSuccessProvider() { + public function moveSuccessProvider() { return array( array('a/b', 'b/b', array('a' => true, 'a/b' => true, 'b' => true, 'b/b' => false), array('a/b' => true)), // older files with special chars can still be renamed to valid names @@ -180,6 +184,90 @@ private function moveTest($source, $destination, $creatables, $updatables, $dele $objectTree->move($source, $destination); } + public function copyDataProvider() { + return [ + // copy into same dir + ['a', 'b', ''], + // copy into same dir + ['a/a', 'a/b', 'a'], + // copy into another dir + ['a', 'sub/a', 'sub'], + ]; + } + + /** + * @dataProvider copyDataProvider + */ + public function testCopy($sourcePath, $targetPath, $targetParent) { + $view = $this->createMock(View::class); + $view->expects($this->once()) + ->method('verifyPath') + ->with($targetParent) + ->will($this->returnValue(true)); + $view->expects($this->once()) + ->method('isCreatable') + ->with($targetParent) + ->will($this->returnValue(true)); + $view->expects($this->once()) + ->method('copy') + ->with($sourcePath, $targetPath) + ->will($this->returnValue(true)); + + $info = new FileInfo('', null, null, [], null); + + $rootDir = new Directory($view, $info); + $objectTree = $this->getMockBuilder(ObjectTree::class) + ->setMethods(['nodeExists', 'getNodeForPath']) + ->setConstructorArgs([$rootDir, $view]) + ->getMock(); + + $objectTree->expects($this->once()) + ->method('getNodeForPath') + ->with($this->identicalTo($sourcePath)) + ->will($this->returnValue(false)); + + /** @var $objectTree \OCA\DAV\Connector\Sabre\ObjectTree */ + $mountManager = Filesystem::getMountManager(); + $objectTree->init($rootDir, $view, $mountManager); + $objectTree->copy($sourcePath, $targetPath); + } + + /** + * @dataProvider copyDataProvider + * @expectedException \Sabre\DAV\Exception\Forbidden + */ + public function testCopyFailNotCreatable($sourcePath, $targetPath, $targetParent) { + $view = $this->createMock(View::class); + $view->expects($this->once()) + ->method('verifyPath') + ->with($targetParent) + ->will($this->returnValue(true)); + $view->expects($this->once()) + ->method('isCreatable') + ->with($targetParent) + ->will($this->returnValue(false)); + $view->expects($this->never()) + ->method('copy'); + + $info = new FileInfo('', null, null, [], null); + + $rootDir = new Directory($view, $info); + $objectTree = $this->getMockBuilder(ObjectTree::class) + ->setMethods(['nodeExists', 'getNodeForPath']) + ->setConstructorArgs([$rootDir, $view]) + ->getMock(); + + $objectTree->expects($this->once()) + ->method('getNodeForPath') + ->with($this->identicalTo($sourcePath)) + ->will($this->returnValue(false)); + + /** @var $objectTree \OCA\DAV\Connector\Sabre\ObjectTree */ + $mountManager = Filesystem::getMountManager(); + $objectTree->init($rootDir, $view, $mountManager); + $objectTree->copy($sourcePath, $targetPath); + } + /** * @dataProvider nodeForPathProvider */ diff --git a/lib/private/Files/View.php b/lib/private/Files/View.php index 70b74a8242e4d..5e581feba6e68 100644 --- a/lib/private/Files/View.php +++ b/lib/private/Files/View.php @@ -88,7 +88,7 @@ class View { /** * @var \OCP\Lock\ILockingProvider */ - private $lockingProvider; + protected $lockingProvider; private $lockingEnabled; From ec8d7010e54138f87a5c17216b6863188db136a4 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Wed, 29 Mar 2017 17:01:04 +0200 Subject: [PATCH 08/16] Accept moving FutureFile into a Directory --- apps/dav/lib/Connector/Sabre/Directory.php | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/apps/dav/lib/Connector/Sabre/Directory.php b/apps/dav/lib/Connector/Sabre/Directory.php index a26420344d040..ddf324ce85184 100644 --- a/apps/dav/lib/Connector/Sabre/Directory.php +++ b/apps/dav/lib/Connector/Sabre/Directory.php @@ -43,6 +43,7 @@ use Sabre\DAV\INode; use Sabre\DAV\Exception\BadRequest; use OC\Files\Mount\MoveableMount; +use Sabre\DAV\IFile; class Directory extends \OCA\DAV\Connector\Sabre\Node implements \Sabre\DAV\ICollection, \Sabre\DAV\IQuota, \Sabre\DAV\IMoveTarget { @@ -344,6 +345,11 @@ public function getQuotaInfo() { */ public function moveInto($targetName, $fullSourcePath, INode $sourceNode) { if (!$sourceNode instanceof Node) { + // it's a file of another kind, like FutureFile + if ($sourceNode instanceof IFile) { + // fallback to default copy+delete handling + return false; + } throw new BadRequest('Incompatible node types'); } From 7b6e4d0dd2dce40fcc7da9de2b7e49b26dfe1914 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Thu, 30 Mar 2017 11:30:37 +0200 Subject: [PATCH 09/16] Fix FutureFile MOVE to keep destination node Sabre usually deletes the target node on MOVE before proceeding with the actual move operation. This fix prevents this to happen in case the source node is a FutureFile. --- apps/dav/lib/Connector/Sabre/Directory.php | 12 ++-- apps/dav/lib/Connector/Sabre/FilesPlugin.php | 41 ++++++++++++ .../unit/Connector/Sabre/FilesPluginTest.php | 63 +++++++++++++++++++ 3 files changed, 112 insertions(+), 4 deletions(-) diff --git a/apps/dav/lib/Connector/Sabre/Directory.php b/apps/dav/lib/Connector/Sabre/Directory.php index ddf324ce85184..9afa63676856c 100644 --- a/apps/dav/lib/Connector/Sabre/Directory.php +++ b/apps/dav/lib/Connector/Sabre/Directory.php @@ -120,9 +120,9 @@ public function createFile($name, $data = null) { if (isset($_SERVER['HTTP_OC_CHUNKED'])) { // exit if we can't create a new file and we don't updatable existing file - $info = \OC_FileChunking::decodeName($name); + $chunkInfo = \OC_FileChunking::decodeName($name); if (!$this->fileView->isCreatable($this->path) && - !$this->fileView->isUpdatable($this->path . '/' . $info['name']) + !$this->fileView->isUpdatable($this->path . '/' . $chunkInfo['name']) ) { throw new \Sabre\DAV\Exception\Forbidden(); } @@ -137,8 +137,12 @@ public function createFile($name, $data = null) { $this->fileView->verifyPath($this->path, $name); $path = $this->fileView->getAbsolutePath($this->path) . '/' . $name; - // using a dummy FileInfo is acceptable here since it will be refreshed after the put is complete - $info = new \OC\Files\FileInfo($path, null, null, array(), null); + // in case the file already exists/overwriting + $info = $this->fileView->getFileInfo($this->path . '/' . $name); + if (!$info) { + // use a dummy FileInfo which is acceptable here since it will be refreshed after the put is complete + $info = new \OC\Files\FileInfo($path, null, null, [], null); + } $node = new \OCA\DAV\Connector\Sabre\File($this->fileView, $info); $node->acquireLock(ILockingProvider::LOCK_SHARED); return $node->put($data); diff --git a/apps/dav/lib/Connector/Sabre/FilesPlugin.php b/apps/dav/lib/Connector/Sabre/FilesPlugin.php index 1b1f43df9eac4..929cd1b0bea26 100644 --- a/apps/dav/lib/Connector/Sabre/FilesPlugin.php +++ b/apps/dav/lib/Connector/Sabre/FilesPlugin.php @@ -45,6 +45,7 @@ use OCP\Files\StorageNotAvailableException; use OCP\IConfig; use OCP\IRequest; +use OCA\DAV\Upload\FutureFile; class FilesPlugin extends ServerPlugin { @@ -177,6 +178,7 @@ public function initialize(\Sabre\DAV\Server $server) { } }); $this->server->on('beforeMove', [$this, 'checkMove']); + $this->server->on('beforeMove', [$this, 'beforeMoveFutureFile']); } /** @@ -436,4 +438,43 @@ public function sendFileIdHeader($filePath, \Sabre\DAV\INode $node = null) { } } } + + /** + * Move handler for future file. + * + * This overrides the default move behavior to prevent Sabre + * to delete the target file before moving. Because deleting would + * lose the file id and metadata. + * + * @param string $path source path + * @param string $destination destination path + * @return bool|void false to stop handling, void to skip this handler + */ + public function beforeMoveFutureFile($path, $destination) { + $sourceNode = $this->tree->getNodeForPath($path); + if (!$sourceNode instanceof FutureFile) { + // skip handling as the source is not a chunked FutureFile + return; + } + + if (!$this->tree->nodeExists($destination)) { + // skip and let the default handler do its work + return; + } + + // do a move manually, skipping Sabre's default "delete" for existing nodes + $this->tree->move($path, $destination); + + // trigger all default events (copied from CorePlugin::move) + $this->server->emit('afterMove', [$path, $destination]); + $this->server->emit('afterUnbind', [$path]); + $this->server->emit('afterBind', [$destination]); + + $response = $this->server->httpResponse; + $response->setHeader('Content-Length', '0'); + $response->setStatus(204); + + return false; + } + } diff --git a/apps/dav/tests/unit/Connector/Sabre/FilesPluginTest.php b/apps/dav/tests/unit/Connector/Sabre/FilesPluginTest.php index 1c9ebdd09b6c0..e0dea49c49b4d 100644 --- a/apps/dav/tests/unit/Connector/Sabre/FilesPluginTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/FilesPluginTest.php @@ -32,6 +32,8 @@ use Sabre\HTTP\RequestInterface; use Sabre\HTTP\ResponseInterface; use Test\TestCase; +use OCA\DAV\Upload\FutureFile; +use OCA\DAV\Connector\Sabre\Directory; /** * Copyright (c) 2015 Vincent Petry @@ -107,6 +109,12 @@ public function setUp() { $this->request, $this->previewManager ); + + $response = $this->getMockBuilder(ResponseInterface::class) + ->disableOriginalConstructor() + ->getMock(); + $this->server->httpResponse = $response; + $this->plugin->initialize($this->server); } @@ -535,4 +543,59 @@ public function testHasPreview() { $this->assertEquals("false", $propFind->get(self::HAS_PREVIEW_PROPERTYNAME)); } + + public function testBeforeMoveFutureFileSkip() { + $node = $this->createMock(Directory::class); + + $this->tree->expects($this->any()) + ->method('getNodeForPath') + ->with('source') + ->will($this->returnValue($node)); + $this->server->httpResponse->expects($this->never()) + ->method('setStatus'); + + $this->assertNull($this->plugin->beforeMoveFutureFile('source', 'target')); + } + + public function testBeforeMoveFutureFileSkipNonExisting() { + $sourceNode = $this->createMock(FutureFile::class); + + $this->tree->expects($this->any()) + ->method('getNodeForPath') + ->with('source') + ->will($this->returnValue($sourceNode)); + $this->tree->expects($this->any()) + ->method('nodeExists') + ->with('target') + ->will($this->returnValue(false)); + $this->server->httpResponse->expects($this->never()) + ->method('setStatus'); + + $this->assertNull($this->plugin->beforeMoveFutureFile('source', 'target')); + } + + public function testBeforeMoveFutureFileMoveIt() { + $sourceNode = $this->createMock(FutureFile::class); + + $this->tree->expects($this->any()) + ->method('getNodeForPath') + ->with('source') + ->will($this->returnValue($sourceNode)); + $this->tree->expects($this->any()) + ->method('nodeExists') + ->with('target') + ->will($this->returnValue(true)); + $this->tree->expects($this->once()) + ->method('move') + ->with('source', 'target'); + + $this->server->httpResponse->expects($this->once()) + ->method('setHeader') + ->with('Content-Length', '0'); + $this->server->httpResponse->expects($this->once()) + ->method('setStatus') + ->with(204); + + $this->assertFalse($this->plugin->beforeMoveFutureFile('source', 'target')); + } } From d379b197d5968e144e5e4e31661ae1f2cf03e2b6 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Thu, 30 Mar 2017 11:31:24 +0200 Subject: [PATCH 10/16] Fix forbidden backslash DAV integration tests --- .../features/webdav-related.feature | 24 +++++++++---------- 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/build/integration/features/webdav-related.feature b/build/integration/features/webdav-related.feature index d0eb3fb4bf4f2..b8ed1c4a77872 100644 --- a/build/integration/features/webdav-related.feature +++ b/build/integration/features/webdav-related.feature @@ -541,40 +541,40 @@ Feature: webdav-related Given using old dav path And user "user0" exists And user "user0" created a folder "/testshare" - When User "user0" moves folder "/testshare" to "/%2F" - Then the HTTP status code should be "403" + When User "user0" moves folder "/testshare" to "/%5C" + Then the HTTP status code should be "400" Scenario: Renaming a folder beginning with a backslash encoded should return an error using old endpoint Given using old dav path And user "user0" exists And user "user0" created a folder "/testshare" - When User "user0" moves folder "/testshare" to "/%2Ftestshare" - Then the HTTP status code should be "403" + When User "user0" moves folder "/testshare" to "/%5Ctestshare" + Then the HTTP status code should be "400" Scenario: Renaming a folder including a backslash encoded should return an error using old endpoint Given using old dav path And user "user0" exists And user "user0" created a folder "/testshare" - When User "user0" moves folder "/testshare" to "/hola%2Fhola" - Then the HTTP status code should be "403" + When User "user0" moves folder "/testshare" to "/hola%5Chola" + Then the HTTP status code should be "400" Scenario: Renaming a folder to a backslash encoded should return an error using new endpoint Given using new dav path And user "user0" exists And user "user0" created a folder "/testshare" - When User "user0" moves folder "/testshare" to "/%2F" - Then the HTTP status code should be "403" + When User "user0" moves folder "/testshare" to "/%5C" + Then the HTTP status code should be "400" Scenario: Renaming a folder beginning with a backslash encoded should return an error using new endpoint Given using new dav path And user "user0" exists And user "user0" created a folder "/testshare" - When User "user0" moves folder "/testshare" to "/%2Ftestshare" - Then the HTTP status code should be "403" + When User "user0" moves folder "/testshare" to "/%5Ctestshare" + Then the HTTP status code should be "400" Scenario: Renaming a folder including a backslash encoded should return an error using new endpoint Given using new dav path And user "user0" exists And user "user0" created a folder "/testshare" - When User "user0" moves folder "/testshare" to "/hola%2Fhola" - Then the HTTP status code should be "403" + When User "user0" moves folder "/testshare" to "/hola%5Chola" + Then the HTTP status code should be "400" From 53deb26778c674760acd0bc3ca08e8fbc607a034 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Thu, 27 Apr 2017 09:15:50 +0200 Subject: [PATCH 11/16] Fix duplicate name of class Signed-off-by: Joas Schilling --- apps/dav/tests/unit/Connector/Sabre/DirectoryTest.php | 6 +++--- apps/dav/tests/unit/Connector/Sabre/ObjectTreeTest.php | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/apps/dav/tests/unit/Connector/Sabre/DirectoryTest.php b/apps/dav/tests/unit/Connector/Sabre/DirectoryTest.php index 1eb3741abf237..fa39cd77274fc 100644 --- a/apps/dav/tests/unit/Connector/Sabre/DirectoryTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/DirectoryTest.php @@ -30,7 +30,7 @@ use OC\Files\FileInfo; use OCA\DAV\Connector\Sabre\Directory; -class TestDoubleFileView extends \OC\Files\View { +class TestViewDirectory extends \OC\Files\View { private $updatables; private $deletables; @@ -386,7 +386,7 @@ public function moveSuccessProvider() { * @param $updatables */ private function moveTest($source, $destination, $updatables, $deletables) { - $view = new TestDoubleFileView($updatables, $deletables); + $view = new TestViewDirectory($updatables, $deletables); $sourceInfo = new FileInfo($source, null, null, [], null); $targetInfo = new FileInfo(dirname($destination), null, null, [], null); @@ -412,7 +412,7 @@ public function testFailingMove() { $updatables = ['a' => true, 'a/b' => true, 'b' => true, 'c/b' => false]; $deletables = ['a/b' => true]; - $view = new TestDoubleFileView($updatables, $deletables); + $view = new TestViewDirectory($updatables, $deletables); $sourceInfo = new FileInfo($source, null, null, [], null); $targetInfo = new FileInfo(dirname($destination), null, null, [], null); diff --git a/apps/dav/tests/unit/Connector/Sabre/ObjectTreeTest.php b/apps/dav/tests/unit/Connector/Sabre/ObjectTreeTest.php index 098d4e2702324..3fae9d626e6c4 100644 --- a/apps/dav/tests/unit/Connector/Sabre/ObjectTreeTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/ObjectTreeTest.php @@ -36,7 +36,7 @@ use OCA\DAV\Connector\Sabre\Directory; use OCA\DAV\Connector\Sabre\ObjectTree; -class TestDoubleFileView extends \OC\Files\View { +class TestViewObjectTree extends \OC\Files\View { public function __construct($creatables, $updatables, $deletables, $canRename = true) { $this->creatables = $creatables; @@ -163,7 +163,7 @@ public function moveSuccessProvider() { * @param $throwsBeforeGetNode */ private function moveTest($source, $destination, $creatables, $updatables, $deletables, $throwsBeforeGetNode = false) { - $view = new TestDoubleFileView($creatables, $updatables, $deletables); + $view = new TestViewObjectTree($creatables, $updatables, $deletables); $info = new FileInfo('', null, null, array(), null); @@ -460,7 +460,7 @@ public function testFailingMove() { $updatables = array('a' => true, 'a/b' => true, 'b' => true, 'b/b' => false); $deletables = array('a/b' => true); - $view = new TestDoubleFileView($updatables, $updatables, $deletables); + $view = new TestViewObjectTree($updatables, $updatables, $deletables); $info = new FileInfo('', null, null, array(), null); From 614bd5c29419c9f45d4fa826539c544a6d7c2e26 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Fri, 24 Feb 2017 11:56:29 +0100 Subject: [PATCH 12/16] Properly handle missing READ permission --- apps/dav/lib/Connector/Sabre/Directory.php | 11 ++++ apps/dav/lib/Connector/Sabre/File.php | 5 ++ apps/dav/lib/Connector/Sabre/FilesPlugin.php | 4 ++ .../unit/Connector/Sabre/DirectoryTest.php | 11 ++-- .../tests/unit/Connector/Sabre/FileTest.php | 19 +++++++ .../unit/Connector/Sabre/FilesPluginTest.php | 55 +++++++++++++++++-- .../Connector/Sabre/FilesReportPluginTest.php | 6 ++ 7 files changed, 100 insertions(+), 11 deletions(-) diff --git a/apps/dav/lib/Connector/Sabre/Directory.php b/apps/dav/lib/Connector/Sabre/Directory.php index 9afa63676856c..579dbbabf4435 100644 --- a/apps/dav/lib/Connector/Sabre/Directory.php +++ b/apps/dav/lib/Connector/Sabre/Directory.php @@ -44,6 +44,7 @@ use Sabre\DAV\Exception\BadRequest; use OC\Files\Mount\MoveableMount; use Sabre\DAV\IFile; +use Sabre\DAV\Exception\NotFound; class Directory extends \OCA\DAV\Connector\Sabre\Node implements \Sabre\DAV\ICollection, \Sabre\DAV\IQuota, \Sabre\DAV\IMoveTarget { @@ -199,6 +200,11 @@ public function createDirectory($name) { * @throws \Sabre\DAV\Exception\ServiceUnavailable */ public function getChild($name, $info = null) { + if (!$this->info->isReadable()) { + // avoid detecting files through this way + throw new NotFound(); + } + $path = $this->path . '/' . $name; if (is_null($info)) { try { @@ -232,12 +238,17 @@ public function getChild($name, $info = null) { * Returns an array with all the child nodes * * @return \Sabre\DAV\INode[] + * @throws \Sabre\DAV\Exception\Locked + * @throws \OCA\DAV\Connector\Sabre\Exception\Forbidden */ public function getChildren() { if (!is_null($this->dirContent)) { return $this->dirContent; } try { + if (!$this->info->isReadable()) { + throw new Forbidden('No read permissions'); + } $folderContent = $this->fileView->getDirectoryContent($this->path); } catch (LockedException $e) { throw new Locked(); diff --git a/apps/dav/lib/Connector/Sabre/File.php b/apps/dav/lib/Connector/Sabre/File.php index 1f878df1564eb..7a8bdb1da750b 100644 --- a/apps/dav/lib/Connector/Sabre/File.php +++ b/apps/dav/lib/Connector/Sabre/File.php @@ -54,6 +54,7 @@ use Sabre\DAV\Exception\NotImplemented; use Sabre\DAV\Exception\ServiceUnavailable; use Sabre\DAV\IFile; +use Sabre\DAV\Exception\NotFound; class File extends Node implements IFile { @@ -307,6 +308,10 @@ private function emitPostHooks($exists, $path = null) { public function get() { //throw exception if encryption is disabled but files are still encrypted try { + if (!$this->info->isReadable()) { + // do a if the file did not exist + throw new NotFound(); + } $res = $this->fileView->fopen(ltrim($this->path, '/'), 'rb'); if ($res === false) { throw new ServiceUnavailable("Could not open file"); diff --git a/apps/dav/lib/Connector/Sabre/FilesPlugin.php b/apps/dav/lib/Connector/Sabre/FilesPlugin.php index 929cd1b0bea26..a4f3f363a5fd3 100644 --- a/apps/dav/lib/Connector/Sabre/FilesPlugin.php +++ b/apps/dav/lib/Connector/Sabre/FilesPlugin.php @@ -286,6 +286,10 @@ public function handleGetProperties(PropFind $propFind, \Sabre\DAV\INode $node) $httpRequest = $this->server->httpRequest; if ($node instanceof \OCA\DAV\Connector\Sabre\Node) { + if (!$node->getFileInfo()->isReadable()) { + // avoid detecting files through this means + throw new NotFound(); + } $propFind->handle(self::FILEID_PROPERTYNAME, function() use ($node) { return $node->getFileId(); diff --git a/apps/dav/tests/unit/Connector/Sabre/DirectoryTest.php b/apps/dav/tests/unit/Connector/Sabre/DirectoryTest.php index fa39cd77274fc..7d4583c801efa 100644 --- a/apps/dav/tests/unit/Connector/Sabre/DirectoryTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/DirectoryTest.php @@ -77,12 +77,11 @@ class DirectoryTest extends \Test\TestCase { protected function setUp() { parent::setUp(); - $this->view = $this->getMockBuilder('OC\Files\View') - ->disableOriginalConstructor() - ->getMock(); - $this->info = $this->getMockBuilder('OC\Files\FileInfo') - ->disableOriginalConstructor() - ->getMock(); + $this->view = $this->createMock('OC\Files\View'); + $this->info = $this->createMock('OC\Files\FileInfo'); + $this->info->expects($this->any()) + ->method('isReadable') + ->will($this->returnValue(true)); } private function getDir($path = '/') { diff --git a/apps/dav/tests/unit/Connector/Sabre/FileTest.php b/apps/dav/tests/unit/Connector/Sabre/FileTest.php index 31344b36463d2..81b7ad27d6a57 100644 --- a/apps/dav/tests/unit/Connector/Sabre/FileTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/FileTest.php @@ -1003,4 +1003,23 @@ public function testGetFopenThrows() { $file->get(); } + + /** + * @expectedException \Sabre\DAV\Exception\NotFound + */ + public function testGetThrowsIfNoPermission() { + $view = $this->getMockBuilder(View::class) + ->setMethods(['fopen']) + ->getMock(); + $view->expects($this->never()) + ->method('fopen'); + + $info = new FileInfo('/test.txt', $this->getMockStorage(), null, [ + 'permissions' => Constants::PERMISSION_CREATE // no read perm + ], null); + + $file = new File($view, $info); + + $file->get(); + } } diff --git a/apps/dav/tests/unit/Connector/Sabre/FilesPluginTest.php b/apps/dav/tests/unit/Connector/Sabre/FilesPluginTest.php index e0dea49c49b4d..739c8f62540da 100644 --- a/apps/dav/tests/unit/Connector/Sabre/FilesPluginTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/FilesPluginTest.php @@ -34,6 +34,7 @@ use Test\TestCase; use OCA\DAV\Upload\FutureFile; use OCA\DAV\Connector\Sabre\Directory; +use OCP\Files\FileInfo; /** * Copyright (c) 2015 Vincent Petry @@ -148,13 +149,15 @@ private function createTestNode($class, $path = '/dummypath') { $node->expects($this->any()) ->method('getDavPermissions') ->will($this->returnValue('DWCKMSR')); + + $fileInfo = $this->createMock(FileInfo::class); + $fileInfo->expects($this->any()) + ->method('isReadable') + ->willReturn(true); + $node->expects($this->any()) ->method('getFileInfo') - ->will($this->returnValue( - $this->getMockBuilder('\OCP\Files\FileInfo') - ->disableOriginalConstructor() - ->getMock() - )); + ->willReturn($fileInfo); return $node; } @@ -313,6 +316,15 @@ public function testGetPropertiesForRootDirectory() { ->getMock(); $node->expects($this->any())->method('getPath')->willReturn('/'); + $fileInfo = $this->createMock(FileInfo::class); + $fileInfo->expects($this->any()) + ->method('isReadable') + ->willReturn(true); + + $node->expects($this->any()) + ->method('getFileInfo') + ->willReturn($fileInfo); + $propFind = new PropFind( '/', [ @@ -329,6 +341,39 @@ public function testGetPropertiesForRootDirectory() { $this->assertEquals('my_fingerprint', $propFind->get(self::DATA_FINGERPRINT_PROPERTYNAME)); } + /** + * @expectedException \Sabre\DAV\Exception\NotFound + */ + public function testGetPropertiesWhenNoPermission() { + /** @var \OCA\DAV\Connector\Sabre\Directory | \PHPUnit_Framework_MockObject_MockObject $node */ + $node = $this->getMockBuilder('\OCA\DAV\Connector\Sabre\Directory') + ->disableOriginalConstructor() + ->getMock(); + $node->expects($this->any())->method('getPath')->willReturn('/'); + + $fileInfo = $this->createMock(FileInfo::class); + $fileInfo->expects($this->any()) + ->method('isReadable') + ->willReturn(false); + + $node->expects($this->any()) + ->method('getFileInfo') + ->willReturn($fileInfo); + + $propFind = new PropFind( + '/test', + [ + self::DATA_FINGERPRINT_PROPERTYNAME, + ], + 0 + ); + + $this->plugin->handleGetProperties( + $propFind, + $node + ); + } + public function testUpdateProps() { $node = $this->createTestNode('\OCA\DAV\Connector\Sabre\File'); diff --git a/apps/dav/tests/unit/Connector/Sabre/FilesReportPluginTest.php b/apps/dav/tests/unit/Connector/Sabre/FilesReportPluginTest.php index 4d8a87b093f28..3ca131dbf6f68 100644 --- a/apps/dav/tests/unit/Connector/Sabre/FilesReportPluginTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/FilesReportPluginTest.php @@ -34,6 +34,7 @@ use OCP\IGroupManager; use OCP\SystemTag\ISystemTagManager; use OCP\ITags; +use OCP\Files\FileInfo; class FilesReportPluginTest extends \Test\TestCase { /** @var \Sabre\DAV\Server|\PHPUnit_Framework_MockObject_MockObject */ @@ -349,6 +350,9 @@ public function testFindNodesByFileIdsSubDir() { public function testPrepareResponses() { $requestedProps = ['{DAV:}getcontentlength', '{http://owncloud.org/ns}fileid', '{DAV:}resourcetype']; + $fileInfo = $this->createMock(FileInfo::class); + $fileInfo->method('isReadable')->willReturn(true); + $node1 = $this->getMockBuilder('\OCA\DAV\Connector\Sabre\Directory') ->disableOriginalConstructor() ->getMock(); @@ -362,6 +366,7 @@ public function testPrepareResponses() { $node1->expects($this->any()) ->method('getPath') ->will($this->returnValue('/node1')); + $node1->method('getFileInfo')->willReturn($fileInfo); $node2->expects($this->once()) ->method('getInternalFileId') ->will($this->returnValue('222')); @@ -371,6 +376,7 @@ public function testPrepareResponses() { $node2->expects($this->any()) ->method('getPath') ->will($this->returnValue('/sub/node2')); + $node2->method('getFileInfo')->willReturn($fileInfo); $config = $this->getMockBuilder('\OCP\IConfig') ->disableOriginalConstructor() From 211a76eff3f23c2037b26bc8ea8e4da10f70e823 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Tue, 28 Feb 2017 10:27:24 +0100 Subject: [PATCH 13/16] Add comment --- apps/dav/lib/Connector/Sabre/Directory.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/apps/dav/lib/Connector/Sabre/Directory.php b/apps/dav/lib/Connector/Sabre/Directory.php index 579dbbabf4435..435f47ee56168 100644 --- a/apps/dav/lib/Connector/Sabre/Directory.php +++ b/apps/dav/lib/Connector/Sabre/Directory.php @@ -247,6 +247,8 @@ public function getChildren() { } try { if (!$this->info->isReadable()) { + // return 403 instead of 404 because a 404 would make + // the caller believe that the collection itself does not exist throw new Forbidden('No read permissions'); } $folderContent = $this->fileView->getDirectoryContent($this->path); From 6fb7d9a865d3265d910be8e34bec44a272f3829e Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Thu, 27 Apr 2017 09:49:37 +0200 Subject: [PATCH 14/16] Don't end the abstract class name with Test.php Signed-off-by: Joas Schilling --- apps/dav/tests/unit/Connector/Sabre/RequestTest/DeleteTest.php | 2 +- .../dav/tests/unit/Connector/Sabre/RequestTest/DownloadTest.php | 2 +- .../Sabre/RequestTest/{RequestTest.php => RequestTestCase.php} | 2 +- apps/dav/tests/unit/Connector/Sabre/RequestTest/UploadTest.php | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) rename apps/dav/tests/unit/Connector/Sabre/RequestTest/{RequestTest.php => RequestTestCase.php} (98%) diff --git a/apps/dav/tests/unit/Connector/Sabre/RequestTest/DeleteTest.php b/apps/dav/tests/unit/Connector/Sabre/RequestTest/DeleteTest.php index 7468e98102002..35fd83f1fe631 100644 --- a/apps/dav/tests/unit/Connector/Sabre/RequestTest/DeleteTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/RequestTest/DeleteTest.php @@ -31,7 +31,7 @@ * * @package OCA\DAV\Tests\unit\Connector\Sabre\RequestTest */ -class DeleteTest extends RequestTest { +class DeleteTest extends RequestTestCase { public function testBasicUpload() { $user = $this->getUniqueID(); $view = $this->setupUser($user, 'pass'); diff --git a/apps/dav/tests/unit/Connector/Sabre/RequestTest/DownloadTest.php b/apps/dav/tests/unit/Connector/Sabre/RequestTest/DownloadTest.php index 8aac99e8c545e..2cb08420f8d39 100644 --- a/apps/dav/tests/unit/Connector/Sabre/RequestTest/DownloadTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/RequestTest/DownloadTest.php @@ -34,7 +34,7 @@ * * @package OCA\DAV\Tests\unit\Connector\Sabre\RequestTest */ -class DownloadTest extends RequestTest { +class DownloadTest extends RequestTestCase { public function testDownload() { $user = $this->getUniqueID(); $view = $this->setupUser($user, 'pass'); diff --git a/apps/dav/tests/unit/Connector/Sabre/RequestTest/RequestTest.php b/apps/dav/tests/unit/Connector/Sabre/RequestTest/RequestTestCase.php similarity index 98% rename from apps/dav/tests/unit/Connector/Sabre/RequestTest/RequestTest.php rename to apps/dav/tests/unit/Connector/Sabre/RequestTest/RequestTestCase.php index 63bd3cf19cc21..50e228b7e846a 100644 --- a/apps/dav/tests/unit/Connector/Sabre/RequestTest/RequestTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/RequestTest/RequestTestCase.php @@ -34,7 +34,7 @@ use Test\Traits\MountProviderTrait; use Test\Traits\UserTrait; -abstract class RequestTest extends TestCase { +abstract class RequestTestCase extends TestCase { use UserTrait; use MountProviderTrait; diff --git a/apps/dav/tests/unit/Connector/Sabre/RequestTest/UploadTest.php b/apps/dav/tests/unit/Connector/Sabre/RequestTest/UploadTest.php index 1db85b1bcaf5b..5376241c61d9e 100644 --- a/apps/dav/tests/unit/Connector/Sabre/RequestTest/UploadTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/RequestTest/UploadTest.php @@ -35,7 +35,7 @@ * * @package OCA\DAV\Tests\unit\Connector\Sabre\RequestTest */ -class UploadTest extends RequestTest { +class UploadTest extends RequestTestCase { public function testBasicUpload() { $user = $this->getUniqueID(); $view = $this->setupUser($user, 'pass'); From a9d06c07d8ee6f8921abc36121a0658e5b536c6c Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Thu, 27 Apr 2017 09:53:55 +0200 Subject: [PATCH 15/16] Fix last unit tests Signed-off-by: Joas Schilling --- .../unit/Connector/Sabre/DirectoryTest.php | 7 +- .../unit/Connector/Sabre/ObjectTreeTest.php | 225 +++--------------- 2 files changed, 32 insertions(+), 200 deletions(-) diff --git a/apps/dav/tests/unit/Connector/Sabre/DirectoryTest.php b/apps/dav/tests/unit/Connector/Sabre/DirectoryTest.php index 7d4583c801efa..f27f67b0aaede 100644 --- a/apps/dav/tests/unit/Connector/Sabre/DirectoryTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/DirectoryTest.php @@ -81,7 +81,7 @@ protected function setUp() { $this->info = $this->createMock('OC\Files\FileInfo'); $this->info->expects($this->any()) ->method('isReadable') - ->will($this->returnValue(true)); + ->willReturn(true); } private function getDir($path = '/') { @@ -221,11 +221,12 @@ public function testGetChildren() { * @expectedException \Sabre\DAV\Exception\Forbidden */ public function testGetChildrenNoPermission() { - $this->info->expects($this->any()) + $info = $this->createMock(FileInfo::class); + $info->expects($this->any()) ->method('isReadable') ->will($this->returnValue(false)); - $dir = new Directory($this->view, $this->info); + $dir = new Directory($this->view, $info); $dir->getChildren(); } diff --git a/apps/dav/tests/unit/Connector/Sabre/ObjectTreeTest.php b/apps/dav/tests/unit/Connector/Sabre/ObjectTreeTest.php index 3fae9d626e6c4..53f60bd0f1c2d 100644 --- a/apps/dav/tests/unit/Connector/Sabre/ObjectTreeTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/ObjectTreeTest.php @@ -36,46 +36,6 @@ use OCA\DAV\Connector\Sabre\Directory; use OCA\DAV\Connector\Sabre\ObjectTree; -class TestViewObjectTree extends \OC\Files\View { - - public function __construct($creatables, $updatables, $deletables, $canRename = true) { - $this->creatables = $creatables; - $this->updatables = $updatables; - $this->deletables = $deletables; - $this->canRename = $canRename; - $this->lockingProvider = \OC::$server->getLockingProvider(); - } - - public function isUpdatable($path) { - return !empty($this->updatables[$path]); - } - - public function isCreatable($path) { - return !empty($this->creatables[$path]); - } - - public function isDeletable($path) { - return !empty($this->deletables[$path]); - } - - public function rename($path1, $path2) { - return $this->canRename; - } - - public function getRelativePath($path) { - return $path; - } - - public function getFileInfo($path, $includeMountPoints = true) { - $objectTreeTest = new ObjectTreeTest(); - return $objectTreeTest->getFileInfoMock( - $this->isCreatable($path), - $this->isUpdatable($path), - $this->isDeletable($path) - ); - } -} - /** * Class ObjectTreeTest * @@ -85,105 +45,6 @@ public function getFileInfo($path, $includeMountPoints = true) { */ class ObjectTreeTest extends \Test\TestCase { - public function getFileInfoMock($create = true, $update = true, $delete = true) { - $mock = $this->getMockBuilder('\OCP\Files\FileInfo') - ->disableOriginalConstructor() - ->getMock(); - $mock - ->expects($this->any()) - ->method('isCreatable') - ->willReturn($create); - $mock - ->expects($this->any()) - ->method('isUpdateable') - ->willReturn($update); - $mock - ->expects($this->any()) - ->method('isDeletable') - ->willReturn($delete); - - return $mock; - } - - /** - * @dataProvider moveFailedProvider - * @expectedException \Sabre\DAV\Exception\Forbidden - */ - public function testMoveFailed($source, $destination, $updatables, $deletables) { - $this->moveTest($source, $destination, $updatables, $updatables, $deletables, true); - } - - /** - * @dataProvider moveSuccessProvider - */ - public function testMoveSuccess($source, $destination, $updatables, $deletables) { - $this->moveTest($source, $destination, $updatables, $updatables, $deletables); - $this->assertTrue(true); - } - - /** - * @dataProvider moveFailedInvalidCharsProvider - * @expectedException \OCA\DAV\Connector\Sabre\Exception\InvalidPath - */ - public function testMoveFailedInvalidChars($source, $destination, $updatables, $deletables) { - $this->moveTest($source, $destination, $updatables, $updatables, $deletables); - } - - public function moveFailedInvalidCharsProvider() { - return array( - array('a/b', 'a/*', array('a' => true, 'a/b' => true, 'a/c*' => false), array()), - ); - } - - public function moveFailedProvider() { - return array( - array('a/b', 'a/c', array('a' => false, 'a/b' => false, 'a/c' => false), array()), - array('a/b', 'b/b', array('a' => false, 'a/b' => false, 'b' => false, 'b/b' => false), array()), - array('a/b', 'b/b', array('a' => false, 'a/b' => true, 'b' => false, 'b/b' => false), array()), - array('a/b', 'b/b', array('a' => true, 'a/b' => true, 'b' => false, 'b/b' => false), array()), - array('a/b', 'b/b', array('a' => true, 'a/b' => true, 'b' => true, 'b/b' => false), array('a/b' => false)), - array('a/b', 'a/c', array('a' => false, 'a/b' => true, 'a/c' => false), array()), - ); - } - - public function moveSuccessProvider() { - return array( - array('a/b', 'b/b', array('a' => true, 'a/b' => true, 'b' => true, 'b/b' => false), array('a/b' => true)), - // older files with special chars can still be renamed to valid names - array('a/b*', 'b/b', array('a' => true, 'a/b*' => true, 'b' => true, 'b/b' => false), array('a/b*' => true)), - ); - } - - /** - * @param $source - * @param $destination - * @param $creatables - * @param $updatables - * @param $deletables - * @param $throwsBeforeGetNode - */ - private function moveTest($source, $destination, $creatables, $updatables, $deletables, $throwsBeforeGetNode = false) { - $view = new TestViewObjectTree($creatables, $updatables, $deletables); - - $info = new FileInfo('', null, null, array(), null); - - $rootDir = new \OCA\DAV\Connector\Sabre\Directory($view, $info); - $objectTree = $this->getMockBuilder('\OCA\DAV\Connector\Sabre\ObjectTree') - ->setMethods(['nodeExists', 'getNodeForPath']) - ->setConstructorArgs([$rootDir, $view]) - ->getMock(); - - $objectTree->expects($throwsBeforeGetNode ? $this->never() : $this->once()) - ->method('getNodeForPath') - ->with($this->identicalTo($source)) - ->will($this->returnValue(false)); - - /** @var $objectTree \OCA\DAV\Connector\Sabre\ObjectTree */ - $mountManager = \OC\Files\Filesystem::getMountManager(); - $objectTree->init($rootDir, $view, $mountManager); - $objectTree->move($source, $destination); - } - public function copyDataProvider() { return [ // copy into same dir @@ -205,15 +66,23 @@ public function testCopy($sourcePath, $targetPath, $targetParent) { ->with($targetParent) ->will($this->returnValue(true)); $view->expects($this->once()) - ->method('isCreatable') - ->with($targetParent) - ->will($this->returnValue(true)); + ->method('file_exists') + ->with($targetPath) + ->willReturn(false); $view->expects($this->once()) ->method('copy') ->with($sourcePath, $targetPath) ->will($this->returnValue(true)); - $info = new FileInfo('', null, null, [], null); + $info = $this->createMock(FileInfo::class); + $info->expects($this->once()) + ->method('isCreatable') + ->willReturn(true); + + $view->expects($this->once()) + ->method('getFileInfo') + ->with($targetParent === '' ? '.' : $targetParent) + ->willReturn($info); $rootDir = new Directory($view, $info); $objectTree = $this->getMockBuilder(ObjectTree::class) @@ -238,18 +107,24 @@ public function testCopy($sourcePath, $targetPath, $targetParent) { */ public function testCopyFailNotCreatable($sourcePath, $targetPath, $targetParent) { $view = $this->createMock(View::class); + $view->expects($this->never()) + ->method('verifyPath'); $view->expects($this->once()) - ->method('verifyPath') - ->with($targetParent) - ->will($this->returnValue(true)); - $view->expects($this->once()) - ->method('isCreatable') - ->with($targetParent) - ->will($this->returnValue(false)); + ->method('file_exists') + ->with($targetPath) + ->willReturn(false); $view->expects($this->never()) ->method('copy'); - $info = new FileInfo('', null, null, [], null); + $info = $this->createMock(FileInfo::class); + $info->expects($this->once()) + ->method('isCreatable') + ->willReturn(false); + + $view->expects($this->once()) + ->method('getFileInfo') + ->with($targetParent === '' ? '.' : $targetParent) + ->willReturn($info); $rootDir = new Directory($view, $info); $objectTree = $this->getMockBuilder(ObjectTree::class) @@ -257,10 +132,8 @@ public function testCopyFailNotCreatable($sourcePath, $targetPath, $targetParent ->setConstructorArgs([$rootDir, $view]) ->getMock(); - $objectTree->expects($this->once()) - ->method('getNodeForPath') - ->with($this->identicalTo($sourcePath)) - ->will($this->returnValue(false)); + $objectTree->expects($this->never()) + ->method('getNodeForPath'); /** @var $objectTree \OCA\DAV\Connector\Sabre\ObjectTree */ $mountManager = Filesystem::getMountManager(); @@ -449,46 +322,4 @@ public function testGetNodeForPathRoot() { $this->assertInstanceOf('\Sabre\DAV\INode', $tree->getNodeForPath($path)); } - - /** - * @expectedException \Sabre\DAV\Exception\Forbidden - * @expectedExceptionMessage Could not copy directory nameOfSourceNode, target exists - */ - public function testFailingMove() { - $source = 'a/b'; - $destination = 'b/b'; - $updatables = array('a' => true, 'a/b' => true, 'b' => true, 'b/b' => false); - $deletables = array('a/b' => true); - - $view = new TestViewObjectTree($updatables, $updatables, $deletables); - - $info = new FileInfo('', null, null, array(), null); - - $rootDir = new \OCA\DAV\Connector\Sabre\Directory($view, $info); - $objectTree = $this->getMockBuilder('\OCA\DAV\Connector\Sabre\ObjectTree') - ->setMethods(['nodeExists', 'getNodeForPath']) - ->setConstructorArgs([$rootDir, $view]) - ->getMock(); - - $sourceNode = $this->getMockBuilder('\Sabre\DAV\ICollection') - ->disableOriginalConstructor() - ->getMock(); - $sourceNode->expects($this->once()) - ->method('getName') - ->will($this->returnValue('nameOfSourceNode')); - - $objectTree->expects($this->once()) - ->method('nodeExists') - ->with($this->identicalTo($destination)) - ->will($this->returnValue(true)); - $objectTree->expects($this->once()) - ->method('getNodeForPath') - ->with($this->identicalTo($source)) - ->will($this->returnValue($sourceNode)); - - /** @var $objectTree \OCA\DAV\Connector\Sabre\ObjectTree */ - $mountManager = \OC\Files\Filesystem::getMountManager(); - $objectTree->init($rootDir, $view, $mountManager); - $objectTree->move($source, $destination); - } } From 5f6153168f89f14c0ada0c98a205cae5eb504d70 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Thu, 27 Apr 2017 15:45:41 +0200 Subject: [PATCH 16/16] Fix class names Signed-off-by: Joas Schilling --- apps/dav/tests/unit/Connector/Sabre/FileTest.php | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/apps/dav/tests/unit/Connector/Sabre/FileTest.php b/apps/dav/tests/unit/Connector/Sabre/FileTest.php index 81b7ad27d6a57..e2666d0de27da 100644 --- a/apps/dav/tests/unit/Connector/Sabre/FileTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/FileTest.php @@ -1008,17 +1008,17 @@ public function testGetFopenThrows() { * @expectedException \Sabre\DAV\Exception\NotFound */ public function testGetThrowsIfNoPermission() { - $view = $this->getMockBuilder(View::class) + $view = $this->getMockBuilder('\OC\Files\View') ->setMethods(['fopen']) ->getMock(); $view->expects($this->never()) ->method('fopen'); - $info = new FileInfo('/test.txt', $this->getMockStorage(), null, [ - 'permissions' => Constants::PERMISSION_CREATE // no read perm + $info = new \OC\Files\FileInfo('/test.txt', $this->getMockStorage(), null, [ + 'permissions' => \OCP\Constants::PERMISSION_CREATE // no read perm ], null); - $file = new File($view, $info); + $file = new \OCA\DAV\Connector\Sabre\File($view, $info); $file->get(); }