From ee1f627155cad4153f3da3160ca6040c137841d3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= Date: Tue, 24 Sep 2013 13:26:12 +0200 Subject: [PATCH 1/3] adding privilege check on move and rename operations --- lib/connector/sabre/node.php | 11 +++++++++++ lib/connector/sabre/objecttree.php | 24 +++++++++++++++++++++++- 2 files changed, 34 insertions(+), 1 deletion(-) diff --git a/lib/connector/sabre/node.php b/lib/connector/sabre/node.php index 0bffa58af7..29b7f9e53a 100644 --- a/lib/connector/sabre/node.php +++ b/lib/connector/sabre/node.php @@ -78,6 +78,11 @@ abstract class OC_Connector_Sabre_Node implements Sabre_DAV_INode, Sabre_DAV_IPr */ public function setName($name) { + // rename is only allowed if the update privilege is granted + if (!\OC\Files\Filesystem::isUpdatable($this->path)) { + throw new \Sabre_DAV_Exception_Forbidden(); + } + list($parentPath, ) = Sabre_DAV_URLUtil::splitPath($this->path); list(, $newName) = Sabre_DAV_URLUtil::splitPath($name); @@ -135,6 +140,12 @@ abstract class OC_Connector_Sabre_Node implements Sabre_DAV_INode, Sabre_DAV_IPr * Even if the modification time is set to a custom value the access time is set to now. */ public function touch($mtime) { + + // touch is only allowed if the update privilege is granted + if (!\OC\Files\Filesystem::isUpdatable($this->path)) { + throw new \Sabre_DAV_Exception_Forbidden(); + } + \OC\Files\Filesystem::touch($this->path, $mtime); } diff --git a/lib/connector/sabre/objecttree.php b/lib/connector/sabre/objecttree.php index acff45ed5e..7accf98c8e 100644 --- a/lib/connector/sabre/objecttree.php +++ b/lib/connector/sabre/objecttree.php @@ -64,7 +64,29 @@ class ObjectTree extends \Sabre_DAV_ObjectTree { list($sourceDir,) = \Sabre_DAV_URLUtil::splitPath($sourcePath); list($destinationDir,) = \Sabre_DAV_URLUtil::splitPath($destinationPath); - Filesystem::rename($sourcePath, $destinationPath); + // check update privileges + if ($sourceDir === $destinationDir) { + // for renaming it's enough to check if the sourcePath can be updated + if (!\OC\Files\Filesystem::isUpdatable($sourcePath)) { + throw new \Sabre_DAV_Exception_Forbidden(); + } + } else { + // for a full move we need update privileges on sourcePath and sourceDir as well as destinationDir + if (!\OC\Files\Filesystem::isUpdatable($sourcePath)) { + throw new \Sabre_DAV_Exception_Forbidden(); + } + if (!\OC\Files\Filesystem::isUpdatable($sourceDir)) { + throw new \Sabre_DAV_Exception_Forbidden(); + } + if (!\OC\Files\Filesystem::isUpdatable($destinationDir)) { + throw new \Sabre_DAV_Exception_Forbidden(); + } + } + + $renameOkay = Filesystem::rename($sourcePath, $destinationPath); + if (!$renameOkay) { + throw new \Sabre_DAV_Exception_Forbidden(''); + } $this->markDirty($sourceDir); $this->markDirty($destinationDir); From b11d8799c17b24e7823de8d8dc171fb78f9b8442 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= Date: Thu, 26 Sep 2013 10:50:15 +0200 Subject: [PATCH 2/3] adding unit tests for ObjectTree::move() --- lib/connector/sabre/objecttree.php | 44 +++++++--- tests/lib/connector/sabre/objecttree.php | 104 +++++++++++++++++++++++ 2 files changed, 134 insertions(+), 14 deletions(-) create mode 100644 tests/lib/connector/sabre/objecttree.php diff --git a/lib/connector/sabre/objecttree.php b/lib/connector/sabre/objecttree.php index 7accf98c8e..80c3840b99 100644 --- a/lib/connector/sabre/objecttree.php +++ b/lib/connector/sabre/objecttree.php @@ -11,6 +11,14 @@ namespace OC\Connector\Sabre; use OC\Files\Filesystem; class ObjectTree extends \Sabre_DAV_ObjectTree { + + /** + * keep this public to allow mock injection during unit test + * + * @var \OC\Files\View + */ + public $fileView; + /** * Returns the INode object for the requested path * @@ -21,14 +29,16 @@ class ObjectTree extends \Sabre_DAV_ObjectTree { public function getNodeForPath($path) { $path = trim($path, '/'); - if (isset($this->cache[$path])) return $this->cache[$path]; + if (isset($this->cache[$path])) { + return $this->cache[$path]; + } // Is it the root node? if (!strlen($path)) { return $this->rootNode; } - $info = Filesystem::getFileInfo($path); + $info = $this->getFileView()->getFileInfo($path); if (!$info) { throw new \Sabre_DAV_Exception_NotFound('File with name ' . $path . ' could not be located'); @@ -65,25 +75,21 @@ class ObjectTree extends \Sabre_DAV_ObjectTree { list($destinationDir,) = \Sabre_DAV_URLUtil::splitPath($destinationPath); // check update privileges - if ($sourceDir === $destinationDir) { - // for renaming it's enough to check if the sourcePath can be updated - if (!\OC\Files\Filesystem::isUpdatable($sourcePath)) { - throw new \Sabre_DAV_Exception_Forbidden(); - } - } else { + $fs = $this->getFileView(); + if (!$fs->isUpdatable($sourcePath)) { + throw new \Sabre_DAV_Exception_Forbidden(); + } + if ($sourceDir !== $destinationDir) { // for a full move we need update privileges on sourcePath and sourceDir as well as destinationDir - if (!\OC\Files\Filesystem::isUpdatable($sourcePath)) { + if (!$fs->isUpdatable($sourceDir)) { throw new \Sabre_DAV_Exception_Forbidden(); } - if (!\OC\Files\Filesystem::isUpdatable($sourceDir)) { - throw new \Sabre_DAV_Exception_Forbidden(); - } - if (!\OC\Files\Filesystem::isUpdatable($destinationDir)) { + if (!$fs->isUpdatable($destinationDir)) { throw new \Sabre_DAV_Exception_Forbidden(); } } - $renameOkay = Filesystem::rename($sourcePath, $destinationPath); + $renameOkay = $fs->rename($sourcePath, $destinationPath); if (!$renameOkay) { throw new \Sabre_DAV_Exception_Forbidden(''); } @@ -123,4 +129,14 @@ class ObjectTree extends \Sabre_DAV_ObjectTree { list($destinationDir,) = \Sabre_DAV_URLUtil::splitPath($destination); $this->markDirty($destinationDir); } + + /** + * @return \OC\Files\View + */ + public function getFileView() { + if (is_null($this->fileView)) { + $this->fileView = \OC\Files\Filesystem::getView(); + } + return $this->fileView; + } } diff --git a/tests/lib/connector/sabre/objecttree.php b/tests/lib/connector/sabre/objecttree.php new file mode 100644 index 0000000000..c920441c8b --- /dev/null +++ b/tests/lib/connector/sabre/objecttree.php @@ -0,0 +1,104 @@ + + * This file is licensed under the Affero General Public License version 3 or + * later. + * See the COPYING-README file. + */ + +namespace Test\OC\Connector\Sabre; + + +use OC_Connector_Sabre_Directory; +use PHPUnit_Framework_TestCase; +use Sabre_DAV_Exception_Forbidden; + +class TestDoubleFileView extends \OC\Files\View{ + + public function __construct($updatables, $canRename = true) { + $this->updatables = $updatables; + $this->canRename = $canRename; + } + + public function isUpdatable($path) { + return $this->updatables[$path]; + } + + public function rename($path1, $path2) { + return $this->canRename; + } +} + +class ObjectTree extends PHPUnit_Framework_TestCase { + + /** + * @dataProvider moveFailedProvider + * @expectedException Sabre_DAV_Exception_Forbidden + */ + public function testMoveFailed($source, $dest, $updatables) { + $rootDir = new OC_Connector_Sabre_Directory(''); + $objectTree = $this->getMock('\OC\Connector\Sabre\ObjectTree', + array('nodeExists', 'getNodeForPath'), + array($rootDir)); + + $objectTree->expects($this->once()) + ->method('getNodeForPath') + ->with($this->identicalTo('a/b')) + ->will($this->returnValue(false)); + + /** @var $objectTree \OC\Connector\Sabre\ObjectTree */ + $objectTree->fileView = new TestDoubleFileView($updatables); + $objectTree->move($source, $dest); + } + + /** + * @dataProvider moveSuccessProvider + */ + public function testMoveSuccess($source, $dest, $updatables) { + $rootDir = new OC_Connector_Sabre_Directory(''); + $objectTree = $this->getMock('\OC\Connector\Sabre\ObjectTree', + array('nodeExists', 'getNodeForPath'), + array($rootDir)); + + $objectTree->expects($this->once()) + ->method('getNodeForPath') + ->with($this->identicalTo('a/b')) + ->will($this->returnValue(false)); + + /** @var $objectTree \OC\Connector\Sabre\ObjectTree */ + $objectTree->fileView = new TestDoubleFileView($updatables); + $objectTree->move($source, $dest); + $this->assertTrue(true); + } + + function moveFailedProvider() { + return array( + array('a/b', 'a/c', array('a' => false, 'a/b' => false, 'a/c' => false)), + array('a/b', 'b/b', array('a' => false, 'a/b' => false, 'b' => false, 'b/b' => false)), + array('a/b', 'b/b', array('a' => false, 'a/b' => true, 'b' => false, 'b/b' => false)), + array('a/b', 'b/b', array('a' => true, 'a/b' => true, 'b' => false, 'b/b' => false)), + ); + } + + function moveSuccessProvider() { + return array( + array('a/b', 'a/c', array('a' => false, 'a/b' => true, 'a/c' => false)), + array('a/b', 'b/b', array('a' => true, 'a/b' => true, 'b' => true, 'b/b' => false)), + ); + } + +// private function buildFileViewMock($updatables) { +// // mock filesysten +// $view = $this->getMock('\OC\Files\View', array('isUpdatable'), array(), '', FALSE); +// +// foreach ($updatables as $path => $updatable) { +// $view->expects($this->any()) +// ->method('isUpdatable') +// ->with($this->identicalTo($path)) +// ->will($this->returnValue($updatable)); +// } +// +// return $view; +// } + +} From 79da35b698a398bef59f83f222de3055ddbb5a92 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= Date: Fri, 27 Sep 2013 13:41:23 +0200 Subject: [PATCH 3/3] code cleanup --- tests/lib/connector/sabre/objecttree.php | 61 +++++++++--------------- 1 file changed, 22 insertions(+), 39 deletions(-) diff --git a/tests/lib/connector/sabre/objecttree.php b/tests/lib/connector/sabre/objecttree.php index c920441c8b..1d76bb5967 100644 --- a/tests/lib/connector/sabre/objecttree.php +++ b/tests/lib/connector/sabre/objecttree.php @@ -36,38 +36,14 @@ class ObjectTree extends PHPUnit_Framework_TestCase { * @expectedException Sabre_DAV_Exception_Forbidden */ public function testMoveFailed($source, $dest, $updatables) { - $rootDir = new OC_Connector_Sabre_Directory(''); - $objectTree = $this->getMock('\OC\Connector\Sabre\ObjectTree', - array('nodeExists', 'getNodeForPath'), - array($rootDir)); - - $objectTree->expects($this->once()) - ->method('getNodeForPath') - ->with($this->identicalTo('a/b')) - ->will($this->returnValue(false)); - - /** @var $objectTree \OC\Connector\Sabre\ObjectTree */ - $objectTree->fileView = new TestDoubleFileView($updatables); - $objectTree->move($source, $dest); + $this->moveTest($source, $dest, $updatables); } /** * @dataProvider moveSuccessProvider */ public function testMoveSuccess($source, $dest, $updatables) { - $rootDir = new OC_Connector_Sabre_Directory(''); - $objectTree = $this->getMock('\OC\Connector\Sabre\ObjectTree', - array('nodeExists', 'getNodeForPath'), - array($rootDir)); - - $objectTree->expects($this->once()) - ->method('getNodeForPath') - ->with($this->identicalTo('a/b')) - ->will($this->returnValue(false)); - - /** @var $objectTree \OC\Connector\Sabre\ObjectTree */ - $objectTree->fileView = new TestDoubleFileView($updatables); - $objectTree->move($source, $dest); + $this->moveTest($source, $dest, $updatables); $this->assertTrue(true); } @@ -87,18 +63,25 @@ class ObjectTree extends PHPUnit_Framework_TestCase { ); } -// private function buildFileViewMock($updatables) { -// // mock filesysten -// $view = $this->getMock('\OC\Files\View', array('isUpdatable'), array(), '', FALSE); -// -// foreach ($updatables as $path => $updatable) { -// $view->expects($this->any()) -// ->method('isUpdatable') -// ->with($this->identicalTo($path)) -// ->will($this->returnValue($updatable)); -// } -// -// return $view; -// } + /** + * @param $source + * @param $dest + * @param $updatables + */ + private function moveTest($source, $dest, $updatables) { + $rootDir = new OC_Connector_Sabre_Directory(''); + $objectTree = $this->getMock('\OC\Connector\Sabre\ObjectTree', + array('nodeExists', 'getNodeForPath'), + array($rootDir)); + + $objectTree->expects($this->once()) + ->method('getNodeForPath') + ->with($this->identicalTo($source)) + ->will($this->returnValue(false)); + + /** @var $objectTree \OC\Connector\Sabre\ObjectTree */ + $objectTree->fileView = new TestDoubleFileView($updatables); + $objectTree->move($source, $dest); + } }