From c2315aa0159628618d58458c1fb4d40287645a06 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Fri, 30 Jan 2015 14:19:23 +0100 Subject: [PATCH 1/5] Fix shared storage permission checks --- apps/files_sharing/lib/sharedstorage.php | 22 ++- apps/files_sharing/tests/sharedstorage.php | 148 +++++++++++++++++++++ 2 files changed, 166 insertions(+), 4 deletions(-) diff --git a/apps/files_sharing/lib/sharedstorage.php b/apps/files_sharing/lib/sharedstorage.php index ccfbebddb2..51b3f36d53 100644 --- a/apps/files_sharing/lib/sharedstorage.php +++ b/apps/files_sharing/lib/sharedstorage.php @@ -294,8 +294,10 @@ class Shared extends \OC\Files\Storage\Common implements ISharedStorage { $relPath1 = $this->getMountPoint() . '/' . $path1; $relPath2 = $this->getMountPoint() . '/' . $path2; + $targetExists = $this->file_exists($path2); + // check for update permissions on the share - if ($this->isUpdatable('')) { + if (($targetExists && $this->isUpdatable('')) || (!$targetExists && $this->isCreatable(''))) { $pathinfo = pathinfo($relPath1); // for part files we need to ask for the owner and path from the parent directory because @@ -343,13 +345,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 2959b9aacf..2b057bd20a 100644 --- a/apps/files_sharing/tests/sharedstorage.php +++ b/apps/files_sharing/tests/sharedstorage.php @@ -199,6 +199,154 @@ class Test_Files_Sharing_Storage extends OCA\Files_sharing\Tests\TestCase { $this->assertTrue($result); } + 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); + } + + 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 allowed as long as target did not exist + $this->assertTrue($user2View->rename($this->folder . '/test-create.txt', $this->folder . '/newtarget.txt')); + $this->assertTrue($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); + } + + 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')); + + // overwriting file directly is allowed + $handle = $user2View->fopen($this->folder . '/existing.txt', 'w'); + $this->assertNotFalse($handle); + fclose($handle); + + // 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); + } + + 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); From a9e6eba018eb8cbd552fef51380cbbfff8c29783 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Wed, 18 Mar 2015 19:22:15 +0100 Subject: [PATCH 2/5] Reenable trashbin after failed rename After a failed rename, the post_rename hook is not called. This quickfix makes sure the trashbin storage logic is reenabled also after a failed rename. --- apps/files_trashbin/lib/storage.php | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/apps/files_trashbin/lib/storage.php b/apps/files_trashbin/lib/storage.php index 175889ef95..d15b136924 100644 --- a/apps/files_trashbin/lib/storage.php +++ b/apps/files_trashbin/lib/storage.php @@ -61,6 +61,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. * From 5ba508b3466fb2021cd1ea9e4c4010875dd0be22 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Thu, 19 Mar 2015 21:18:48 +0100 Subject: [PATCH 3/5] Fix permission checks in Sabre connector This fixes moving files in and out of shared folders with some exotic permission combinations. --- lib/private/connector/sabre/objecttree.php | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/lib/private/connector/sabre/objecttree.php b/lib/private/connector/sabre/objecttree.php index 3705aa8058..1de0ee73ec 100644 --- a/lib/private/connector/sabre/objecttree.php +++ b/lib/private/connector/sabre/objecttree.php @@ -158,8 +158,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); @@ -173,14 +174,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(); } From 5f7b3a4dbe2b62d315cced584988a0b1929959e0 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Thu, 19 Mar 2015 21:55:56 +0100 Subject: [PATCH 4/5] Rename must be possible with update-only permission and this as long as the rename is done within the same folder. --- apps/files_sharing/lib/sharedstorage.php | 40 ++++++++++++++---------- 1 file changed, 23 insertions(+), 17 deletions(-) diff --git a/apps/files_sharing/lib/sharedstorage.php b/apps/files_sharing/lib/sharedstorage.php index 51b3f36d53..a3f306f26a 100644 --- a/apps/files_sharing/lib/sharedstorage.php +++ b/apps/files_sharing/lib/sharedstorage.php @@ -293,28 +293,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); + $isPartFile = (isset($pathinfo['extension']) && $pathinfo['extension'] === 'part'); $targetExists = $this->file_exists($path2); - - // check for update permissions on the share - if (($targetExists && $this->isUpdatable('')) || (!$targetExists && $this->isCreatable(''))) { - - $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); + $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) { From eef5851a6750da9fb15446af09dd8f385131de73 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Fri, 20 Mar 2015 11:30:51 +0100 Subject: [PATCH 5/5] Fix share permission related unit tests --- apps/files_sharing/tests/sharedstorage.php | 22 +++++++++++++--------- tests/lib/connector/sabre/objecttree.php | 4 ++-- 2 files changed, 15 insertions(+), 11 deletions(-) diff --git a/apps/files_sharing/tests/sharedstorage.php b/apps/files_sharing/tests/sharedstorage.php index 2b057bd20a..f840b9b964 100644 --- a/apps/files_sharing/tests/sharedstorage.php +++ b/apps/files_sharing/tests/sharedstorage.php @@ -199,7 +199,7 @@ class Test_Files_Sharing_Storage extends OCA\Files_sharing\Tests\TestCase { $this->assertTrue($result); } - function testFopenWithReadOnlyPermission() { + 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, @@ -230,7 +230,7 @@ class Test_Files_Sharing_Storage extends OCA\Files_sharing\Tests\TestCase { $this->assertTrue($result); } - function testFopenWithCreateOnlyPermission() { + 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, @@ -250,9 +250,9 @@ class Test_Files_Sharing_Storage extends OCA\Files_sharing\Tests\TestCase { $this->assertNotFalse($handle); fclose($handle); - // rename file allowed as long as target did not exist - $this->assertTrue($user2View->rename($this->folder . '/test-create.txt', $this->folder . '/newtarget.txt')); - $this->assertTrue($user2View->file_exists($this->folder . '/newtarget.txt')); + // 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')); @@ -274,7 +274,7 @@ class Test_Files_Sharing_Storage extends OCA\Files_sharing\Tests\TestCase { $this->assertTrue($result); } - function testFopenWithUpdateOnlyPermission() { + public function testFopenWithUpdateOnlyPermission() { $this->view->file_put_contents($this->folder . '/existing.txt', 'foo'); $fileinfoFolder = $this->view->getFileInfo($this->folder); @@ -301,13 +301,17 @@ class Test_Files_Sharing_Storage extends OCA\Files_sharing\Tests\TestCase { $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.txt', 'w'); + $handle = $user2View->fopen($this->folder . '/existing-renamed.txt', 'w'); $this->assertNotFalse($handle); fclose($handle); // delete forbidden - $this->assertFalse($user2View->unlink($this->folder . '/existing.txt')); + $this->assertFalse($user2View->unlink($this->folder . '/existing-renamed.txt')); //cleanup self::loginHelper(self::TEST_FILES_SHARING_API_USER1); @@ -316,7 +320,7 @@ class Test_Files_Sharing_Storage extends OCA\Files_sharing\Tests\TestCase { $this->assertTrue($result); } - function testFopenWithDeleteOnlyPermission() { + 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, 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)),