From 1ee56c702de8498bdc17bc7e93ff4c24c7b5590b Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Tue, 6 Oct 2015 15:36:54 +0200 Subject: [PATCH] [WEBDAV] check if delete of source is allowed on move Fixes #5251 If we perform a move we need to make sure first that the source can be deleted. Else the dest might be cleared but the move will fail later. * Added unit tests Eventually we need more and better checking here. --- lib/private/connector/sabre/filesplugin.php | 34 ++++++++++- lib/private/connector/sabre/serverfactory.php | 2 +- tests/lib/connector/sabre/filesplugin.php | 61 ++++++++++++++++++- 3 files changed, 91 insertions(+), 6 deletions(-) diff --git a/lib/private/connector/sabre/filesplugin.php b/lib/private/connector/sabre/filesplugin.php index 84620f454a..ab7f6884a5 100644 --- a/lib/private/connector/sabre/filesplugin.php +++ b/lib/private/connector/sabre/filesplugin.php @@ -64,10 +64,20 @@ class FilesPlugin extends \Sabre\DAV\ServerPlugin { private $isPublic; /** - * @param \Sabre\DAV\Tree $tree + * @var \OC\Files\View */ - public function __construct(\Sabre\DAV\Tree $tree, $isPublic = false) { + private $fileView; + + /** + * @param \Sabre\DAV\Tree $tree + * @param \OC\Files\View $view + * @param bool $isPublic + */ + public function __construct(\Sabre\DAV\Tree $tree, + \OC\Files\View $view, + $isPublic = false) { $this->tree = $tree; + $this->fileView = $view; $this->isPublic = $isPublic; } @@ -106,6 +116,26 @@ class FilesPlugin extends \Sabre\DAV\ServerPlugin { fclose($body); } }); + $this->server->on('beforeMove', [$this, 'checkMove']); + } + + /** + * Plugin that checks if a move can actually be performed. + * @param string $source source path + * @param string $destination destination path + * @throws \Sabre\DAV\Exception\Forbidden + */ + function checkMove($source, $destination) { + list($sourceDir,) = \Sabre\HTTP\URLUtil::splitPath($source); + list($destinationDir,) = \Sabre\HTTP\URLUtil::splitPath($destination); + + if ($sourceDir !== $destinationDir) { + $sourceFileInfo = $this->fileView->getFileInfo($source); + + if (!$sourceFileInfo->isDeletable()) { + throw new \Sabre\DAV\Exception\Forbidden($source . " cannot be deleted"); + } + } } /** diff --git a/lib/private/connector/sabre/serverfactory.php b/lib/private/connector/sabre/serverfactory.php index a0c32c1da5..893e29fd41 100644 --- a/lib/private/connector/sabre/serverfactory.php +++ b/lib/private/connector/sabre/serverfactory.php @@ -72,7 +72,6 @@ class ServerFactory { $server->addPlugin(new \Sabre\DAV\Auth\Plugin($authBackend, $defaults->getName())); // FIXME: The following line is a workaround for legacy components relying on being able to send a GET to / $server->addPlugin(new \OC\Connector\Sabre\DummyGetResponsePlugin()); - $server->addPlugin(new \OC\Connector\Sabre\FilesPlugin($objectTree)); $server->addPlugin(new \OC\Connector\Sabre\ExceptionLoggerPlugin('webdav', $this->logger)); $server->addPlugin(new \OC\Connector\Sabre\LockPlugin($objectTree)); $server->addPlugin(new \OC\Connector\Sabre\ListenerPlugin($this->dispatcher)); @@ -91,6 +90,7 @@ class ServerFactory { } $objectTree->init($root, $view, $this->mountManager); + $server->addPlugin(new \OC\Connector\Sabre\FilesPlugin($objectTree, $view)); $server->addPlugin(new \OC\Connector\Sabre\QuotaPlugin($view)); if($this->userSession->isLoggedIn()) { diff --git a/tests/lib/connector/sabre/filesplugin.php b/tests/lib/connector/sabre/filesplugin.php index a4cf9f7bfb..0aa62cab17 100644 --- a/tests/lib/connector/sabre/filesplugin.php +++ b/tests/lib/connector/sabre/filesplugin.php @@ -31,13 +31,24 @@ class FilesPlugin extends \Test\TestCase { */ private $plugin; + /** + * @var \OC\Files\View + */ + private $view; + public function setUp() { parent::setUp(); - $this->server = new \Sabre\DAV\Server(); + $this->server = $this->getMockBuilder('\Sabre\DAV\Server') + ->disableOriginalConstructor() + ->getMock(); $this->tree = $this->getMockBuilder('\Sabre\DAV\Tree') ->disableOriginalConstructor() ->getMock(); - $this->plugin = new \OC\Connector\Sabre\FilesPlugin($this->tree); + $this->view = $this->getMockBuilder('\OC\Files\View') + ->disableOriginalConstructor() + ->getMock(); + + $this->plugin = new \OC\Connector\Sabre\FilesPlugin($this->tree, $this->view); $this->plugin->initialize($this->server); } @@ -104,7 +115,7 @@ class FilesPlugin extends \Test\TestCase { } public function testGetPublicPermissions() { - $this->plugin = new \OC\Connector\Sabre\FilesPlugin($this->tree, true); + $this->plugin = new \OC\Connector\Sabre\FilesPlugin($this->tree, $this->view, true); $this->plugin->initialize($this->server); $propFind = new \Sabre\DAV\PropFind( @@ -196,4 +207,48 @@ class FilesPlugin extends \Test\TestCase { $this->assertEquals(200, $result[self::GETETAG_PROPERTYNAME]); } + /** + * Testcase from https://github.com/owncloud/core/issues/5251 + * + * |-FolderA + * |-text.txt + * |-test.txt + * + * FolderA is an incomming shared folder and there are no delete permissions. + * Thus moving /FolderA/test.txt to /test.txt should fail already on that check + * + * @expectedException \Sabre\DAV\Exception\Forbidden + * @expectedExceptionMessage FolderA/test.txt cannot be deleted + */ + public function testMoveSrcNotDeletable() { + $fileInfoFolderATestTXT = $this->getMockBuilder('\OCP\Files\FileInfo') + ->disableOriginalConstructor() + ->getMock(); + $fileInfoFolderATestTXT->expects($this->once()) + ->method('isDeletable') + ->willReturn(false); + + $this->view->expects($this->once()) + ->method('getFileInfo') + ->with('FolderA/test.txt') + ->willReturn($fileInfoFolderATestTXT); + + $this->plugin->checkMove('FolderA/test.txt', 'test.txt'); + } + + public function testMoveSrcDeletable() { + $fileInfoFolderATestTXT = $this->getMockBuilder('\OCP\Files\FileInfo') + ->disableOriginalConstructor() + ->getMock(); + $fileInfoFolderATestTXT->expects($this->once()) + ->method('isDeletable') + ->willReturn(true); + + $this->view->expects($this->once()) + ->method('getFileInfo') + ->with('FolderA/test.txt') + ->willReturn($fileInfoFolderATestTXT); + + $this->plugin->checkMove('FolderA/test.txt', 'test.txt'); + } }