diff --git a/apps/files_sharing/lib/sharedstorage.php b/apps/files_sharing/lib/sharedstorage.php index e5968aeb06..c7285b1d5c 100644 --- a/apps/files_sharing/lib/sharedstorage.php +++ b/apps/files_sharing/lib/sharedstorage.php @@ -299,26 +299,34 @@ class Shared extends \OC\Files\Storage\Common implements ISharedStorage { // we need the paths relative to data/user/files $relPath1 = $this->getMountPoint() . '/' . $path1; $relPath2 = $this->getMountPoint() . '/' . $path2; + $pathinfo = pathinfo($relPath1); - // check for update permissions on the share - if ($this->isUpdatable('')) { - - $pathinfo = pathinfo($relPath1); - // for part files we need to ask for the owner and path from the parent directory because - // the file cache doesn't return any results for part files - if (isset($pathinfo['extension']) && $pathinfo['extension'] === 'part') { - list($user1, $path1) = \OCA\Files_Sharing\Helper::getUidAndFilename($pathinfo['dirname']); - $path1 = $path1 . '/' . $pathinfo['basename']; - } else { - list($user1, $path1) = \OCA\Files_Sharing\Helper::getUidAndFilename($relPath1); + $isPartFile = (isset($pathinfo['extension']) && $pathinfo['extension'] === 'part'); + $targetExists = $this->file_exists($path2); + $sameFolder = (dirname($relPath1) === dirname($relPath2)); + if ($targetExists || ($sameFolder && !$isPartFile)) { + // note that renaming a share mount point is always allowed + if (!$this->isUpdatable('')) { + return false; + } + } else { + if (!$this->isCreatable('')) { + return false; } - $targetFilename = basename($relPath2); - list($user2, $path2) = \OCA\Files_Sharing\Helper::getUidAndFilename(dirname($relPath2)); - $rootView = new \OC\Files\View(''); - return $rootView->rename('/' . $user1 . '/files/' . $path1, '/' . $user2 . '/files/' . $path2 . '/' . $targetFilename); } - return false; + // for part files we need to ask for the owner and path from the parent directory because + // the file cache doesn't return any results for part files + if ($isPartFile) { + list($user1, $path1) = \OCA\Files_Sharing\Helper::getUidAndFilename($pathinfo['dirname']); + $path1 = $path1 . '/' . $pathinfo['basename']; + } else { + list($user1, $path1) = \OCA\Files_Sharing\Helper::getUidAndFilename($relPath1); + } + $targetFilename = basename($relPath2); + list($user2, $path2) = \OCA\Files_Sharing\Helper::getUidAndFilename(dirname($relPath2)); + $rootView = new \OC\Files\View(''); + return $rootView->rename('/' . $user1 . '/files/' . $path1, '/' . $user2 . '/files/' . $path2 . '/' . $targetFilename); } public function copy($path1, $path2) { @@ -349,13 +357,25 @@ class Shared extends \OC\Files\Storage\Common implements ISharedStorage { case 'xb': case 'a': case 'ab': - $exists = $this->file_exists($path); - if ($exists && !$this->isUpdatable($path)) { + $creatable = $this->isCreatable($path); + $updatable = $this->isUpdatable($path); + // if neither permissions given, no need to continue + if (!$creatable && !$updatable) { return false; } - if (!$exists && !$this->isCreatable(dirname($path))) { + + $exists = $this->file_exists($path); + // if a file exists, updatable permissions are required + if ($exists && !$updatable) { return false; } + + // part file is allowed if !$creatable but the final file is $updatable + if (pathinfo($path, PATHINFO_EXTENSION) !== 'part') { + if (!$exists && !$creatable) { + return false; + } + } } $info = array( 'target' => $this->getMountPoint() . $path, diff --git a/apps/files_sharing/tests/sharedstorage.php b/apps/files_sharing/tests/sharedstorage.php index 4a589a5a73..a8ce92775d 100644 --- a/apps/files_sharing/tests/sharedstorage.php +++ b/apps/files_sharing/tests/sharedstorage.php @@ -202,6 +202,158 @@ class Test_Files_Sharing_Storage extends OCA\Files_sharing\Tests\TestCase { $this->assertTrue($result); } + public function testFopenWithReadOnlyPermission() { + $this->view->file_put_contents($this->folder . '/existing.txt', 'foo'); + $fileinfoFolder = $this->view->getFileInfo($this->folder); + $result = \OCP\Share::shareItem('folder', $fileinfoFolder['fileid'], \OCP\Share::SHARE_TYPE_USER, + self::TEST_FILES_SHARING_API_USER2, \OCP\Constants::PERMISSION_READ); + $this->assertTrue($result); + + self::loginHelper(self::TEST_FILES_SHARING_API_USER2); + $user2View = new \OC\Files\View('/' . self::TEST_FILES_SHARING_API_USER2 . '/files'); + + // part file should be forbidden + $handle = $user2View->fopen($this->folder . '/test.txt.part', 'w'); + $this->assertFalse($handle); + + // regular file forbidden + $handle = $user2View->fopen($this->folder . '/test.txt', 'w'); + $this->assertFalse($handle); + + // rename forbidden + $this->assertFalse($user2View->rename($this->folder . '/existing.txt', $this->folder . '/existing2.txt')); + + // delete forbidden + $this->assertFalse($user2View->unlink($this->folder . '/existing.txt')); + + //cleanup + self::loginHelper(self::TEST_FILES_SHARING_API_USER1); + $result = \OCP\Share::unshare('folder', $fileinfoFolder['fileid'], \OCP\Share::SHARE_TYPE_USER, + self::TEST_FILES_SHARING_API_USER2); + $this->assertTrue($result); + } + + public function testFopenWithCreateOnlyPermission() { + $this->view->file_put_contents($this->folder . '/existing.txt', 'foo'); + $fileinfoFolder = $this->view->getFileInfo($this->folder); + $result = \OCP\Share::shareItem('folder', $fileinfoFolder['fileid'], \OCP\Share::SHARE_TYPE_USER, + self::TEST_FILES_SHARING_API_USER2, \OCP\Constants::PERMISSION_READ | \OCP\Constants::PERMISSION_CREATE); + $this->assertTrue($result); + + self::loginHelper(self::TEST_FILES_SHARING_API_USER2); + $user2View = new \OC\Files\View('/' . self::TEST_FILES_SHARING_API_USER2 . '/files'); + + // create part file allowed + $handle = $user2View->fopen($this->folder . '/test.txt.part', 'w'); + $this->assertNotFalse($handle); + fclose($handle); + + // create regular file allowed + $handle = $user2View->fopen($this->folder . '/test-create.txt', 'w'); + $this->assertNotFalse($handle); + fclose($handle); + + // rename file never allowed + $this->assertFalse($user2View->rename($this->folder . '/test-create.txt', $this->folder . '/newtarget.txt')); + $this->assertFalse($user2View->file_exists($this->folder . '/newtarget.txt')); + + // rename file not allowed if target exists + $this->assertFalse($user2View->rename($this->folder . '/newtarget.txt', $this->folder . '/existing.txt')); + + // overwriting file not allowed + $handle = $user2View->fopen($this->folder . '/existing.txt', 'w'); + $this->assertFalse($handle); + + // overwrite forbidden (no update permission) + $this->assertFalse($user2View->rename($this->folder . '/test.txt.part', $this->folder . '/existing.txt')); + + // delete forbidden + $this->assertFalse($user2View->unlink($this->folder . '/existing.txt')); + + //cleanup + self::loginHelper(self::TEST_FILES_SHARING_API_USER1); + $result = \OCP\Share::unshare('folder', $fileinfoFolder['fileid'], \OCP\Share::SHARE_TYPE_USER, + self::TEST_FILES_SHARING_API_USER2); + $this->assertTrue($result); + } + + public function testFopenWithUpdateOnlyPermission() { + $this->view->file_put_contents($this->folder . '/existing.txt', 'foo'); + $fileinfoFolder = $this->view->getFileInfo($this->folder); + + $result = \OCP\Share::shareItem('folder', $fileinfoFolder['fileid'], \OCP\Share::SHARE_TYPE_USER, + self::TEST_FILES_SHARING_API_USER2, \OCP\Constants::PERMISSION_READ | \OCP\Constants::PERMISSION_UPDATE); + $this->assertTrue($result); + + self::loginHelper(self::TEST_FILES_SHARING_API_USER2); + $user2View = new \OC\Files\View('/' . self::TEST_FILES_SHARING_API_USER2 . '/files'); + + // create part file allowed + $handle = $user2View->fopen($this->folder . '/test.txt.part', 'w'); + $this->assertNotFalse($handle); + fclose($handle); + + // create regular file not allowed + $handle = $user2View->fopen($this->folder . '/test-create.txt', 'w'); + $this->assertFalse($handle); + + // rename part file not allowed to non-existing file + $this->assertFalse($user2View->rename($this->folder . '/test.txt.part', $this->folder . '/nonexist.txt')); + + // rename part file allowed to target existing file + $this->assertTrue($user2View->rename($this->folder . '/test.txt.part', $this->folder . '/existing.txt')); + $this->assertTrue($user2View->file_exists($this->folder . '/existing.txt')); + + // rename regular file allowed + $this->assertTrue($user2View->rename($this->folder . '/existing.txt', $this->folder . '/existing-renamed.txt')); + $this->assertTrue($user2View->file_exists($this->folder . '/existing-renamed.txt')); + + // overwriting file directly is allowed + $handle = $user2View->fopen($this->folder . '/existing-renamed.txt', 'w'); + $this->assertNotFalse($handle); + fclose($handle); + + // delete forbidden + $this->assertFalse($user2View->unlink($this->folder . '/existing-renamed.txt')); + + //cleanup + self::loginHelper(self::TEST_FILES_SHARING_API_USER1); + $result = \OCP\Share::unshare('folder', $fileinfoFolder['fileid'], \OCP\Share::SHARE_TYPE_USER, + self::TEST_FILES_SHARING_API_USER2); + $this->assertTrue($result); + } + + public function testFopenWithDeleteOnlyPermission() { + $this->view->file_put_contents($this->folder . '/existing.txt', 'foo'); + $fileinfoFolder = $this->view->getFileInfo($this->folder); + $result = \OCP\Share::shareItem('folder', $fileinfoFolder['fileid'], \OCP\Share::SHARE_TYPE_USER, + self::TEST_FILES_SHARING_API_USER2, \OCP\Constants::PERMISSION_READ | \OCP\Constants::PERMISSION_DELETE); + $this->assertTrue($result); + + self::loginHelper(self::TEST_FILES_SHARING_API_USER2); + $user2View = new \OC\Files\View('/' . self::TEST_FILES_SHARING_API_USER2 . '/files'); + + // part file should be forbidden + $handle = $user2View->fopen($this->folder . '/test.txt.part', 'w'); + $this->assertFalse($handle); + + // regular file forbidden + $handle = $user2View->fopen($this->folder . '/test.txt', 'w'); + $this->assertFalse($handle); + + // rename forbidden + $this->assertFalse($user2View->rename($this->folder . '/existing.txt', $this->folder . '/existing2.txt')); + + // delete allowed + $this->assertTrue($user2View->unlink($this->folder . '/existing.txt')); + + //cleanup + self::loginHelper(self::TEST_FILES_SHARING_API_USER1); + $result = \OCP\Share::unshare('folder', $fileinfoFolder['fileid'], \OCP\Share::SHARE_TYPE_USER, + self::TEST_FILES_SHARING_API_USER2); + $this->assertTrue($result); + } + function testMountSharesOtherUser() { $folderInfo = $this->view->getFileInfo($this->folder); $fileInfo = $this->view->getFileInfo($this->filename); diff --git a/apps/files_trashbin/lib/storage.php b/apps/files_trashbin/lib/storage.php index 72788aa5ba..3f474d71da 100644 --- a/apps/files_trashbin/lib/storage.php +++ b/apps/files_trashbin/lib/storage.php @@ -62,6 +62,22 @@ class Storage extends Wrapper { self::$disableTrash = false; } + /** + * Rename path1 to path2 by calling the wrapped storage. + * + * @param string $path1 first path + * @param string $path2 second path + */ + public function rename($path1, $path2) { + $result = $this->storage->rename($path1, $path2); + if ($result === false) { + // when rename failed, the post_rename hook isn't triggered, + // but we still want to reenable the trash logic + self::$disableTrash = false; + } + return $result; + } + /** * Deletes the given file by moving it into the trashbin. * diff --git a/lib/private/connector/sabre/objecttree.php b/lib/private/connector/sabre/objecttree.php index 79ae19522c..999750fd78 100644 --- a/lib/private/connector/sabre/objecttree.php +++ b/lib/private/connector/sabre/objecttree.php @@ -175,8 +175,9 @@ class ObjectTree extends \Sabre\DAV\Tree { throw new \Sabre\DAV\Exception\ServiceUnavailable('filesystem not setup'); } + $targetNodeExists = $this->nodeExists($destinationPath); $sourceNode = $this->getNodeForPath($sourcePath); - if ($sourceNode instanceof \Sabre\DAV\ICollection and $this->nodeExists($destinationPath)) { + if ($sourceNode instanceof \Sabre\DAV\ICollection && $targetNodeExists) { throw new \Sabre\DAV\Exception\Forbidden('Could not copy directory ' . $sourceNode . ', target exists'); } list($sourceDir,) = \Sabre\HTTP\URLUtil::splitPath($sourcePath); @@ -190,14 +191,22 @@ class ObjectTree extends \Sabre\DAV\Tree { } try { - // check update privileges - if (!$this->fileView->isUpdatable($sourcePath) && !$isMovableMount) { - throw new \Sabre\DAV\Exception\Forbidden(); - } - if ($sourceDir !== $destinationDir) { + $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(); } diff --git a/tests/lib/connector/sabre/objecttree.php b/tests/lib/connector/sabre/objecttree.php index d2702027b0..53e53f1e07 100644 --- a/tests/lib/connector/sabre/objecttree.php +++ b/tests/lib/connector/sabre/objecttree.php @@ -69,7 +69,7 @@ class ObjectTree extends \Test\TestCase { function moveFailedInvalidCharsProvider() { return array( - array('a/b', 'a/*', array('a' => false, 'a/b' => true, 'a/c*' => false), array()), + array('a/b', 'a/*', array('a' => true, 'a/b' => true, 'a/c*' => false), array()), ); } @@ -80,12 +80,12 @@ class ObjectTree extends \Test\TestCase { 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()), ); } function moveSuccessProvider() { return array( - array('a/b', 'a/c', array('a' => false, 'a/b' => true, 'a/c' => false), 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)),