From 9ab17c95c0746cced8a4f36bec58dc98ee229ef3 Mon Sep 17 00:00:00 2001 From: Sergio Bertolin Date: Mon, 27 Mar 2017 10:27:23 +0000 Subject: Added test about checking file id after a move --- build/integration/features/bootstrap/WebDav.php | 35 +++++++++++++++++++++-- build/integration/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 3a018a2d0fa..9242b80ac13 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 @@ trait WebDav { } + /** + * @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 6aee59036d3..457d92d52a6 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" -- cgit v1.2.3 From c30feafaa2f2fb86469f5c38a6e20ed3f3042873 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Mon, 27 Mar 2017 18:50:02 +0200 Subject: 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 457d92d52a6..231f49f4398 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" + -- cgit v1.2.3 From 9bff66e68d9b2a5712eae406b54d103b51a8c7e9 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Mon, 27 Mar 2017 18:53:06 +0200 Subject: 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 231f49f4398..14033a69a72 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" -- cgit v1.2.3 From a761d4cce725435036c980667109ddb25126aa31 Mon Sep 17 00:00:00 2001 From: Sergio Bertolin Date: Wed, 29 Mar 2017 09:21:13 +0000 Subject: Added test cases from core 16825 --- build/integration/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 14033a69a72..d0eb3fb4bf4 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" -- cgit v1.2.3 From 0a9f7730d0daeed565af28ad2e1528606bab6ba7 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Wed, 29 Mar 2017 11:04:27 +0200 Subject: 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 25cca40a889..a26420344d0 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\Forbidden; 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 @@ class Directory extends \OCA\DAV\Connector\Sabre\Node 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 @@ class Directory extends \OCA\DAV\Connector\Sabre\Node } } 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 @@ class Directory extends \OCA\DAV\Connector\Sabre\Node $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 @@ class Directory extends \OCA\DAV\Connector\Sabre\Node } } + /** + * 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; + } } -- cgit v1.2.3 From 82b967d3f9f661b784f5ba97d5a69d0b40a1c7be Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Wed, 29 Mar 2017 11:16:41 +0200 Subject: 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 554a7ad86ca..acc6dcc3be3 100644 --- a/apps/dav/lib/Connector/Sabre/ObjectTree.php +++ b/apps/dav/lib/Connector/Sabre/ObjectTree.php @@ -184,104 +184,6 @@ class ObjectTree extends \Sabre\DAV\Tree { } - /** - * 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. * -- cgit v1.2.3 From 642b4331a6c0aec3a6a7d0350f3582df7caafc6b Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Wed, 29 Mar 2017 12:15:42 +0200 Subject: Moved unit tests from ObjectTree::move to Directory --- .../tests/unit/Connector/Sabre/DirectoryTest.php | 170 ++++++++++++++++++++- .../tests/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 18f91bbd8c9..1eb3741abf2 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 @@ class DirectoryTest extends \Test\TestCase { ->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 @@ class DirectoryTest extends \Test\TestCase { ->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 @@ class DirectoryTest extends \Test\TestCase { $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 @@ class DirectoryTest extends \Test\TestCase { ->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 @@ class DirectoryTest extends \Test\TestCase { $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 @@ class DirectoryTest extends \Test\TestCase { ->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 @@ class DirectoryTest extends \Test\TestCase { ->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 17cb598bf6e..098d4e27023 100644 --- a/apps/dav/tests/unit/Connector/Sabre/ObjectTreeTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/ObjectTreeTest.php @@ -30,7 +30,11 @@ namespace OCA\DAV\Tests\unit\Connector\Sabre; 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 @@ class ObjectTreeTest extends \Test\TestCase { $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 @@ class ObjectTreeTest extends \Test\TestCase { ); } - 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 @@ class ObjectTreeTest extends \Test\TestCase { $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 70b74a8242e..5e581feba6e 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; -- cgit v1.2.3 From ec8d7010e54138f87a5c17216b6863188db136a4 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Wed, 29 Mar 2017 17:01:04 +0200 Subject: 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 a26420344d0..ddf324ce851 100644 --- a/apps/dav/lib/Connector/Sabre/Directory.php +++ b/apps/dav/lib/Connector/Sabre/Directory.php @@ -43,6 +43,7 @@ use Sabre\DAV\Exception\ServiceUnavailable; 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 @@ class Directory extends \OCA\DAV\Connector\Sabre\Node */ 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'); } -- cgit v1.2.3 From 7b6e4d0dd2dce40fcc7da9de2b7e49b26dfe1914 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Thu, 30 Mar 2017 11:30:37 +0200 Subject: 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 ++++++++++++++ .../tests/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 ddf324ce851..9afa6367685 100644 --- a/apps/dav/lib/Connector/Sabre/Directory.php +++ b/apps/dav/lib/Connector/Sabre/Directory.php @@ -120,9 +120,9 @@ class Directory extends \OCA\DAV\Connector\Sabre\Node 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 @@ class Directory extends \OCA\DAV\Connector\Sabre\Node $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 1b1f43df9ea..929cd1b0bea 100644 --- a/apps/dav/lib/Connector/Sabre/FilesPlugin.php +++ b/apps/dav/lib/Connector/Sabre/FilesPlugin.php @@ -45,6 +45,7 @@ use \Sabre\HTTP\ResponseInterface; use OCP\Files\StorageNotAvailableException; use OCP\IConfig; use OCP\IRequest; +use OCA\DAV\Upload\FutureFile; class FilesPlugin extends ServerPlugin { @@ -177,6 +178,7 @@ class FilesPlugin extends ServerPlugin { } }); $this->server->on('beforeMove', [$this, 'checkMove']); + $this->server->on('beforeMove', [$this, 'beforeMoveFutureFile']); } /** @@ -436,4 +438,43 @@ class FilesPlugin extends ServerPlugin { } } } + + /** + * 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 1c9ebdd09b6..e0dea49c49b 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\DAV\PropPatch; 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 @@ class FilesPluginTest extends TestCase { $this->request, $this->previewManager ); + + $response = $this->getMockBuilder(ResponseInterface::class) + ->disableOriginalConstructor() + ->getMock(); + $this->server->httpResponse = $response; + $this->plugin->initialize($this->server); } @@ -535,4 +543,59 @@ class FilesPluginTest extends TestCase { $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')); + } } -- cgit v1.2.3 From d379b197d5968e144e5e4e31661ae1f2cf03e2b6 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Thu, 30 Mar 2017 11:31:24 +0200 Subject: Fix forbidden backslash DAV integration tests --- build/integration/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 d0eb3fb4bf4..b8ed1c4a778 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" -- cgit v1.2.3 From 53deb26778c674760acd0bc3ca08e8fbc607a034 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Thu, 27 Apr 2017 09:15:50 +0200 Subject: 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 1eb3741abf2..fa39cd77274 100644 --- a/apps/dav/tests/unit/Connector/Sabre/DirectoryTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/DirectoryTest.php @@ -30,7 +30,7 @@ use OCP\Files\ForbiddenException; 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 @@ class DirectoryTest extends \Test\TestCase { * @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 @@ class DirectoryTest extends \Test\TestCase { $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 098d4e27023..3fae9d626e6 100644 --- a/apps/dav/tests/unit/Connector/Sabre/ObjectTreeTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/ObjectTreeTest.php @@ -36,7 +36,7 @@ use OC\Files\View; 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 @@ class ObjectTreeTest extends \Test\TestCase { * @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 @@ class ObjectTreeTest extends \Test\TestCase { $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); -- cgit v1.2.3 From 614bd5c29419c9f45d4fa826539c544a6d7c2e26 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Fri, 24 Feb 2017 11:56:29 +0100 Subject: 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 ++ .../tests/unit/Connector/Sabre/DirectoryTest.php | 11 ++--- apps/dav/tests/unit/Connector/Sabre/FileTest.php | 19 ++++++++ .../tests/unit/Connector/Sabre/FilesPluginTest.php | 55 ++++++++++++++++++++-- .../unit/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 9afa6367685..579dbbabf44 100644 --- a/apps/dav/lib/Connector/Sabre/Directory.php +++ b/apps/dav/lib/Connector/Sabre/Directory.php @@ -44,6 +44,7 @@ use Sabre\DAV\INode; 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 @@ class Directory extends \OCA\DAV\Connector\Sabre\Node * @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 @@ class Directory extends \OCA\DAV\Connector\Sabre\Node * 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 1f878df1564..7a8bdb1da75 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\Forbidden; 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 @@ class File extends Node implements IFile { 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 929cd1b0bea..a4f3f363a5f 100644 --- a/apps/dav/lib/Connector/Sabre/FilesPlugin.php +++ b/apps/dav/lib/Connector/Sabre/FilesPlugin.php @@ -286,6 +286,10 @@ class FilesPlugin extends ServerPlugin { $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 fa39cd77274..7d4583c801e 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 31344b36463..81b7ad27d6a 100644 --- a/apps/dav/tests/unit/Connector/Sabre/FileTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/FileTest.php @@ -1003,4 +1003,23 @@ class FileTest extends \Test\TestCase { $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 e0dea49c49b..739c8f62540 100644 --- a/apps/dav/tests/unit/Connector/Sabre/FilesPluginTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/FilesPluginTest.php @@ -34,6 +34,7 @@ use Sabre\HTTP\ResponseInterface; 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 @@ class FilesPluginTest extends TestCase { $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 @@ class FilesPluginTest extends TestCase { ->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 @@ class FilesPluginTest extends TestCase { $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 4d8a87b093f..3ca131dbf6f 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\Files\Folder; 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 @@ class FilesReportPluginTest extends \Test\TestCase { 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 @@ class FilesReportPluginTest extends \Test\TestCase { $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 @@ class FilesReportPluginTest extends \Test\TestCase { $node2->expects($this->any()) ->method('getPath') ->will($this->returnValue('/sub/node2')); + $node2->method('getFileInfo')->willReturn($fileInfo); $config = $this->getMockBuilder('\OCP\IConfig') ->disableOriginalConstructor() -- cgit v1.2.3 From 211a76eff3f23c2037b26bc8ea8e4da10f70e823 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Tue, 28 Feb 2017 10:27:24 +0100 Subject: 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 579dbbabf44..435f47ee561 100644 --- a/apps/dav/lib/Connector/Sabre/Directory.php +++ b/apps/dav/lib/Connector/Sabre/Directory.php @@ -247,6 +247,8 @@ class Directory extends \OCA\DAV\Connector\Sabre\Node } 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); -- cgit v1.2.3 From 6fb7d9a865d3265d910be8e34bec44a272f3829e Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Thu, 27 Apr 2017 09:49:37 +0200 Subject: Don't end the abstract class name with Test.php Signed-off-by: Joas Schilling --- .../Connector/Sabre/RequestTest/DeleteTest.php | 2 +- .../Connector/Sabre/RequestTest/DownloadTest.php | 2 +- .../Connector/Sabre/RequestTest/RequestTest.php | 149 --------------------- .../Sabre/RequestTest/RequestTestCase.php | 149 +++++++++++++++++++++ .../Connector/Sabre/RequestTest/UploadTest.php | 2 +- 5 files changed, 152 insertions(+), 152 deletions(-) delete mode 100644 apps/dav/tests/unit/Connector/Sabre/RequestTest/RequestTest.php create mode 100644 apps/dav/tests/unit/Connector/Sabre/RequestTest/RequestTestCase.php diff --git a/apps/dav/tests/unit/Connector/Sabre/RequestTest/DeleteTest.php b/apps/dav/tests/unit/Connector/Sabre/RequestTest/DeleteTest.php index 7468e981020..35fd83f1fe6 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 @@ use OCP\AppFramework\Http; * * @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 8aac99e8c54..2cb08420f8d 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 @@ use OCP\Lock\ILockingProvider; * * @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/RequestTest.php deleted file mode 100644 index 63bd3cf19cc..00000000000 --- a/apps/dav/tests/unit/Connector/Sabre/RequestTest/RequestTest.php +++ /dev/null @@ -1,149 +0,0 @@ - - * @author Lukas Reschke - * @author Robin Appelman - * @author Roeland Jago Douma - * @author Thomas Müller - * - * @license AGPL-3.0 - * - * This code is free software: you can redistribute it and/or modify - * it under the terms of the GNU Affero General Public License, version 3, - * as published by the Free Software Foundation. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * GNU Affero General Public License for more details. - * - * You should have received a copy of the GNU Affero General Public License, version 3, - * along with this program. If not, see - * - */ - -namespace OCA\DAV\Tests\unit\Connector\Sabre\RequestTest; - -use OCA\DAV\Connector\Sabre\Server; -use OCA\DAV\Connector\Sabre\ServerFactory; -use OC\Files\View; -use Sabre\HTTP\Request; -use Test\TestCase; -use Test\Traits\MountProviderTrait; -use Test\Traits\UserTrait; - -abstract class RequestTest extends TestCase { - use UserTrait; - use MountProviderTrait; - - /** - * @var \OCA\DAV\Connector\Sabre\ServerFactory - */ - protected $serverFactory; - - protected function getStream($string) { - $stream = fopen('php://temp', 'r+'); - fwrite($stream, $string); - fseek($stream, 0); - return $stream; - } - - protected function setUp() { - parent::setUp(); - - unset($_SERVER['HTTP_OC_CHUNKED']); - - $this->serverFactory = new ServerFactory( - \OC::$server->getConfig(), - \OC::$server->getLogger(), - \OC::$server->getDatabaseConnection(), - \OC::$server->getUserSession(), - \OC::$server->getMountManager(), - \OC::$server->getTagManager(), - $this->getMockBuilder('\OCP\IRequest') - ->disableOriginalConstructor() - ->getMock(), - \OC::$server->getPreviewManager() - ); - } - - protected function setupUser($name, $password) { - $this->createUser($name, $password); - $tmpFolder = \OC::$server->getTempManager()->getTemporaryFolder(); - $this->registerMount($name, '\OC\Files\Storage\Local', '/' . $name, ['datadir' => $tmpFolder]); - $this->loginAsUser($name); - return new View('/' . $name . '/files'); - } - - /** - * @param \OC\Files\View $view the view to run the webdav server against - * @param string $user - * @param string $password - * @param string $method - * @param string $url - * @param resource|string|null $body - * @param array|null $headers - * @return \Sabre\HTTP\Response - * @throws \Exception - */ - protected function request($view, $user, $password, $method, $url, $body = null, $headers = null) { - if (is_string($body)) { - $body = $this->getStream($body); - } - $this->logout(); - $exceptionPlugin = new ExceptionPlugin('webdav', null); - $server = $this->getSabreServer($view, $user, $password, $exceptionPlugin); - $request = new Request($method, $url, $headers, $body); - - // since sabre catches all exceptions we need to save them and throw them from outside the sabre server - - $originalServer = $_SERVER; - - if (is_array($headers)) { - foreach ($headers as $header => $value) { - $_SERVER['HTTP_' . strtoupper(str_replace('-', '_', $header))] = $value; - } - } - - $result = $this->makeRequest($server, $request); - - foreach ($exceptionPlugin->getExceptions() as $exception) { - throw $exception; - } - $_SERVER = $originalServer; - return $result; - } - - /** - * @param Server $server - * @param Request $request - * @return \Sabre\HTTP\Response - */ - protected function makeRequest(Server $server, Request $request) { - $sapi = new Sapi($request); - $server->sapi = $sapi; - $server->httpRequest = $request; - $server->exec(); - return $sapi->getResponse(); - } - - /** - * @param View $view - * @param string $user - * @param string $password - * @param ExceptionPlugin $exceptionPlugin - * @return Server - */ - protected function getSabreServer(View $view, $user, $password, ExceptionPlugin $exceptionPlugin) { - $authBackend = new Auth($user, $password); - - $server = $this->serverFactory->createServer('/', 'dummy', $authBackend, function () use ($view) { - return $view; - }); - $server->addPlugin($exceptionPlugin); - - return $server; - } -} diff --git a/apps/dav/tests/unit/Connector/Sabre/RequestTest/RequestTestCase.php b/apps/dav/tests/unit/Connector/Sabre/RequestTest/RequestTestCase.php new file mode 100644 index 00000000000..50e228b7e84 --- /dev/null +++ b/apps/dav/tests/unit/Connector/Sabre/RequestTest/RequestTestCase.php @@ -0,0 +1,149 @@ + + * @author Lukas Reschke + * @author Robin Appelman + * @author Roeland Jago Douma + * @author Thomas Müller + * + * @license AGPL-3.0 + * + * This code is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License, version 3, + * as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License, version 3, + * along with this program. If not, see + * + */ + +namespace OCA\DAV\Tests\unit\Connector\Sabre\RequestTest; + +use OCA\DAV\Connector\Sabre\Server; +use OCA\DAV\Connector\Sabre\ServerFactory; +use OC\Files\View; +use Sabre\HTTP\Request; +use Test\TestCase; +use Test\Traits\MountProviderTrait; +use Test\Traits\UserTrait; + +abstract class RequestTestCase extends TestCase { + use UserTrait; + use MountProviderTrait; + + /** + * @var \OCA\DAV\Connector\Sabre\ServerFactory + */ + protected $serverFactory; + + protected function getStream($string) { + $stream = fopen('php://temp', 'r+'); + fwrite($stream, $string); + fseek($stream, 0); + return $stream; + } + + protected function setUp() { + parent::setUp(); + + unset($_SERVER['HTTP_OC_CHUNKED']); + + $this->serverFactory = new ServerFactory( + \OC::$server->getConfig(), + \OC::$server->getLogger(), + \OC::$server->getDatabaseConnection(), + \OC::$server->getUserSession(), + \OC::$server->getMountManager(), + \OC::$server->getTagManager(), + $this->getMockBuilder('\OCP\IRequest') + ->disableOriginalConstructor() + ->getMock(), + \OC::$server->getPreviewManager() + ); + } + + protected function setupUser($name, $password) { + $this->createUser($name, $password); + $tmpFolder = \OC::$server->getTempManager()->getTemporaryFolder(); + $this->registerMount($name, '\OC\Files\Storage\Local', '/' . $name, ['datadir' => $tmpFolder]); + $this->loginAsUser($name); + return new View('/' . $name . '/files'); + } + + /** + * @param \OC\Files\View $view the view to run the webdav server against + * @param string $user + * @param string $password + * @param string $method + * @param string $url + * @param resource|string|null $body + * @param array|null $headers + * @return \Sabre\HTTP\Response + * @throws \Exception + */ + protected function request($view, $user, $password, $method, $url, $body = null, $headers = null) { + if (is_string($body)) { + $body = $this->getStream($body); + } + $this->logout(); + $exceptionPlugin = new ExceptionPlugin('webdav', null); + $server = $this->getSabreServer($view, $user, $password, $exceptionPlugin); + $request = new Request($method, $url, $headers, $body); + + // since sabre catches all exceptions we need to save them and throw them from outside the sabre server + + $originalServer = $_SERVER; + + if (is_array($headers)) { + foreach ($headers as $header => $value) { + $_SERVER['HTTP_' . strtoupper(str_replace('-', '_', $header))] = $value; + } + } + + $result = $this->makeRequest($server, $request); + + foreach ($exceptionPlugin->getExceptions() as $exception) { + throw $exception; + } + $_SERVER = $originalServer; + return $result; + } + + /** + * @param Server $server + * @param Request $request + * @return \Sabre\HTTP\Response + */ + protected function makeRequest(Server $server, Request $request) { + $sapi = new Sapi($request); + $server->sapi = $sapi; + $server->httpRequest = $request; + $server->exec(); + return $sapi->getResponse(); + } + + /** + * @param View $view + * @param string $user + * @param string $password + * @param ExceptionPlugin $exceptionPlugin + * @return Server + */ + protected function getSabreServer(View $view, $user, $password, ExceptionPlugin $exceptionPlugin) { + $authBackend = new Auth($user, $password); + + $server = $this->serverFactory->createServer('/', 'dummy', $authBackend, function () use ($view) { + return $view; + }); + $server->addPlugin($exceptionPlugin); + + return $server; + } +} diff --git a/apps/dav/tests/unit/Connector/Sabre/RequestTest/UploadTest.php b/apps/dav/tests/unit/Connector/Sabre/RequestTest/UploadTest.php index 1db85b1bcaf..5376241c61d 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 @@ use OCP\Lock\ILockingProvider; * * @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'); -- cgit v1.2.3 From a9d06c07d8ee6f8921abc36121a0658e5b536c6c Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Thu, 27 Apr 2017 09:53:55 +0200 Subject: Fix last unit tests Signed-off-by: Joas Schilling --- .../tests/unit/Connector/Sabre/DirectoryTest.php | 7 +- .../tests/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 7d4583c801e..f27f67b0aae 100644 --- a/apps/dav/tests/unit/Connector/Sabre/DirectoryTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/DirectoryTest.php @@ -81,7 +81,7 @@ class DirectoryTest extends \Test\TestCase { $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 @@ class DirectoryTest extends \Test\TestCase { * @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 3fae9d626e6..53f60bd0f1c 100644 --- a/apps/dav/tests/unit/Connector/Sabre/ObjectTreeTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/ObjectTreeTest.php @@ -36,46 +36,6 @@ use OC\Files\View; 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 @@ class TestViewObjectTree extends \OC\Files\View { */ 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 @@ class ObjectTreeTest extends \Test\TestCase { ->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 @@ class ObjectTreeTest extends \Test\TestCase { */ 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 @@ class ObjectTreeTest extends \Test\TestCase { ->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 @@ class ObjectTreeTest extends \Test\TestCase { $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); - } } -- cgit v1.2.3 From 5f6153168f89f14c0ada0c98a205cae5eb504d70 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Thu, 27 Apr 2017 15:45:41 +0200 Subject: 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 81b7ad27d6a..e2666d0de27 100644 --- a/apps/dav/tests/unit/Connector/Sabre/FileTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/FileTest.php @@ -1008,17 +1008,17 @@ class FileTest extends \Test\TestCase { * @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(); } -- cgit v1.2.3