From 14299e44e2b350da2f5d8cb88e40f803633e0112 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 7 Sep 2016 11:10:48 +0200 Subject: [PATCH 1/3] Only require CREATE permissions when the file does not exist yet --- apps/dav/lib/Connector/Sabre/ObjectTree.php | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/apps/dav/lib/Connector/Sabre/ObjectTree.php b/apps/dav/lib/Connector/Sabre/ObjectTree.php index e872ea5827..9f6eb8a85b 100644 --- a/apps/dav/lib/Connector/Sabre/ObjectTree.php +++ b/apps/dav/lib/Connector/Sabre/ObjectTree.php @@ -205,7 +205,11 @@ class ObjectTree extends \Sabre\DAV\Tree { $infoDestination = $this->fileView->getFileInfo(dirname($destinationPath)); $infoSource = $this->fileView->getFileInfo($sourcePath); - $destinationPermission = $infoDestination && $infoDestination->isUpdateable(); + if ($this->fileView->file_exists($destinationPath)) { + $destinationPermission = $infoDestination && $infoDestination->isUpdateable(); + } else { + $destinationPermission = $infoDestination && $infoDestination->isCreatable(); + } $sourcePermission = $infoSource && $infoSource->isDeletable(); if (!$destinationPermission || !$sourcePermission) { @@ -298,7 +302,12 @@ class ObjectTree extends \Sabre\DAV\Tree { $info = $this->fileView->getFileInfo(dirname($destination)); - if ($info && !$info->isUpdateable()) { + if ($this->fileView->file_exists($destination)) { + $destinationPermission = $info && $info->isUpdateable(); + } else { + $destinationPermission = $info && $info->isCreatable(); + } + if (!$destinationPermission) { throw new Forbidden('No permissions to copy object.'); } From 42b9eed5dcde89e1f2323efcdca0cbe859725631 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 7 Sep 2016 11:22:57 +0200 Subject: [PATCH 2/3] UPDATE permissions qualify for renaming a node --- apps/dav/lib/Connector/Sabre/ObjectTree.php | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/apps/dav/lib/Connector/Sabre/ObjectTree.php b/apps/dav/lib/Connector/Sabre/ObjectTree.php index 9f6eb8a85b..af1cf79e1d 100644 --- a/apps/dav/lib/Connector/Sabre/ObjectTree.php +++ b/apps/dav/lib/Connector/Sabre/ObjectTree.php @@ -204,13 +204,18 @@ class ObjectTree extends \Sabre\DAV\Tree { } $infoDestination = $this->fileView->getFileInfo(dirname($destinationPath)); - $infoSource = $this->fileView->getFileInfo($sourcePath); - if ($this->fileView->file_exists($destinationPath)) { - $destinationPermission = $infoDestination && $infoDestination->isUpdateable(); + if (dirname($destinationPath) === dirname($sourcePath)) { + $sourcePermission = $infoDestination && $infoDestination->isUpdateable(); + $destinationPermission = $sourcePermission; } else { - $destinationPermission = $infoDestination && $infoDestination->isCreatable(); + $infoSource = $this->fileView->getFileInfo($sourcePath); + if ($this->fileView->file_exists($destinationPath)) { + $destinationPermission = $infoDestination && $infoDestination->isUpdateable(); + } else { + $destinationPermission = $infoDestination && $infoDestination->isCreatable(); + } + $sourcePermission = $infoSource && $infoSource->isDeletable(); } - $sourcePermission = $infoSource && $infoSource->isDeletable(); if (!$destinationPermission || !$sourcePermission) { throw new Forbidden('No permissions to move object.'); From 2e407732ba51f08ae55da85a331a123941e119df Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 7 Sep 2016 18:26:44 +0200 Subject: [PATCH 3/3] Fix tests --- .../unit/Connector/Sabre/ObjectTreeTest.php | 44 ++++++++++++------- 1 file changed, 28 insertions(+), 16 deletions(-) diff --git a/apps/dav/tests/unit/Connector/Sabre/ObjectTreeTest.php b/apps/dav/tests/unit/Connector/Sabre/ObjectTreeTest.php index 0b942aa1bc..17cb598bf6 100644 --- a/apps/dav/tests/unit/Connector/Sabre/ObjectTreeTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/ObjectTreeTest.php @@ -34,7 +34,8 @@ use OC\Files\Storage\Temporary; class TestDoubleFileView extends \OC\Files\View { - public function __construct($updatables, $deletables, $canRename = true) { + public function __construct($creatables, $updatables, $deletables, $canRename = true) { + $this->creatables = $creatables; $this->updatables = $updatables; $this->deletables = $deletables; $this->canRename = $canRename; @@ -42,15 +43,15 @@ class TestDoubleFileView extends \OC\Files\View { } public function isUpdatable($path) { - return $this->updatables[$path]; + return !empty($this->updatables[$path]); } public function isCreatable($path) { - return $this->updatables[$path]; + return !empty($this->creatables[$path]); } public function isDeletable($path) { - return $this->deletables[$path]; + return !empty($this->deletables[$path]); } public function rename($path1, $path2) { @@ -63,7 +64,11 @@ class TestDoubleFileView extends \OC\Files\View { public function getFileInfo($path, $includeMountPoints = true) { $objectTreeTest = new ObjectTreeTest(); - return $objectTreeTest->getFileInfoMock(); + return $objectTreeTest->getFileInfoMock( + $this->isCreatable($path), + $this->isUpdatable($path), + $this->isDeletable($path) + ); } } @@ -76,18 +81,22 @@ class TestDoubleFileView extends \OC\Files\View { */ class ObjectTreeTest extends \Test\TestCase { - public function getFileInfoMock() { + public function getFileInfoMock($create = true, $update = true, $delete = true) { $mock = $this->getMockBuilder('\OCP\Files\FileInfo') ->disableOriginalConstructor() ->getMock(); $mock ->expects($this->any()) - ->method('isDeletable') - ->willReturn(true); + ->method('isCreatable') + ->willReturn($create); $mock ->expects($this->any()) ->method('isUpdateable') - ->willReturn(true); + ->willReturn($update); + $mock + ->expects($this->any()) + ->method('isDeletable') + ->willReturn($delete); return $mock; } @@ -97,14 +106,14 @@ class ObjectTreeTest extends \Test\TestCase { * @expectedException \Sabre\DAV\Exception\Forbidden */ public function testMoveFailed($source, $destination, $updatables, $deletables) { - $this->moveTest($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, $deletables); + $this->moveTest($source, $destination, $updatables, $updatables, $deletables); $this->assertTrue(true); } @@ -113,7 +122,7 @@ class ObjectTreeTest extends \Test\TestCase { * @expectedException \OCA\DAV\Connector\Sabre\Exception\InvalidPath */ public function testMoveFailedInvalidChars($source, $destination, $updatables, $deletables) { - $this->moveTest($source, $destination, $updatables, $deletables); + $this->moveTest($source, $destination, $updatables, $updatables, $deletables); } function moveFailedInvalidCharsProvider() { @@ -144,10 +153,13 @@ class ObjectTreeTest extends \Test\TestCase { /** * @param $source * @param $destination + * @param $creatables * @param $updatables + * @param $deletables + * @param $throwsBeforeGetNode */ - private function moveTest($source, $destination, $updatables, $deletables) { - $view = new TestDoubleFileView($updatables, $deletables); + private function moveTest($source, $destination, $creatables, $updatables, $deletables, $throwsBeforeGetNode = false) { + $view = new TestDoubleFileView($creatables, $updatables, $deletables); $info = new FileInfo('', null, null, array(), null); @@ -157,7 +169,7 @@ class ObjectTreeTest extends \Test\TestCase { ->setConstructorArgs([$rootDir, $view]) ->getMock(); - $objectTree->expects($this->once()) + $objectTree->expects($throwsBeforeGetNode ? $this->never() : $this->once()) ->method('getNodeForPath') ->with($this->identicalTo($source)) ->will($this->returnValue(false)); @@ -360,7 +372,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, $deletables); + $view = new TestDoubleFileView($updatables, $updatables, $deletables); $info = new FileInfo('', null, null, array(), null);