From ed981294f11bd59733e0d121cbf737e16275b666 Mon Sep 17 00:00:00 2001 From: Bjoern Schiessle Date: Tue, 8 Apr 2014 19:57:07 +0200 Subject: fix share api tests --- apps/files_sharing/tests/api.php | 70 ++++++++-------------------------------- 1 file changed, 14 insertions(+), 56 deletions(-) (limited to 'apps/files_sharing/tests/api.php') diff --git a/apps/files_sharing/tests/api.php b/apps/files_sharing/tests/api.php index c7a848315ac..6354d1099bb 100644 --- a/apps/files_sharing/tests/api.php +++ b/apps/files_sharing/tests/api.php @@ -324,10 +324,10 @@ class Test_Files_Sharing_Api extends Test_Files_Sharing_Base { $this->assertTrue(is_string($result)); $testValues=array( - array('query' => 'Shared/' . $this->folder, - 'expectedResult' => '/Shared' . $this->folder . $this->filename), - array('query' => 'Shared/' . $this->folder . $this->subfolder, - 'expectedResult' => '/Shared' . $this->folder . $this->subfolder . $this->filename), + array('query' => $this->folder, + 'expectedResult' => $this->folder . $this->filename), + array('query' => $this->folder . $this->subfolder, + 'expectedResult' => $this->folder . $this->subfolder . $this->filename), ); foreach ($testValues as $value) { @@ -382,7 +382,7 @@ class Test_Files_Sharing_Api extends Test_Files_Sharing_Base { // share was successful? $this->assertTrue(is_string($result)); - $_GET['path'] = '/Shared'; + $_GET['path'] = '/'; $_GET['subfiles'] = 'true'; $result = Share\Api::getAllShares(array()); @@ -395,7 +395,7 @@ class Test_Files_Sharing_Api extends Test_Files_Sharing_Base { // we should get exactly one result $this->assertEquals(1, count($data)); - $expectedPath = '/Shared' . $this->subfolder; + $expectedPath = $this->subfolder; $this->assertEquals($expectedPath, $data[0]['path']); // cleanup @@ -444,7 +444,7 @@ class Test_Files_Sharing_Api extends Test_Files_Sharing_Base { $this->assertTrue(is_string($result)); - $_GET['path'] = '/Shared'; + $_GET['path'] = '/'; $_GET['subfiles'] = 'true'; $result = Share\Api::getAllShares(array()); @@ -457,7 +457,7 @@ class Test_Files_Sharing_Api extends Test_Files_Sharing_Base { // we should get exactly one result $this->assertEquals(1, count($data)); - $expectedPath = '/Shared' . $this->subsubfolder; + $expectedPath = $this->subsubfolder; $this->assertEquals($expectedPath, $data[0]['path']); @@ -512,8 +512,8 @@ class Test_Files_Sharing_Api extends Test_Files_Sharing_Base { $this->assertTrue(is_string($result)); - // ask for shared/subfolder - $expectedPath1 = '/Shared' . $this->subfolder; + // ask for subfolder + $expectedPath1 = $this->subfolder; $_GET['path'] = $expectedPath1; $result1 = Share\Api::getAllShares(array()); @@ -524,8 +524,8 @@ class Test_Files_Sharing_Api extends Test_Files_Sharing_Base { $data1 = $result1->getData(); $share1 = reset($data1); - // ask for shared/folder/subfolder - $expectedPath2 = '/Shared' . $this->folder . $this->subfolder; + // ask for folder/subfolder + $expectedPath2 = $this->folder . $this->subfolder; $_GET['path'] = $expectedPath2; $result2 = Share\Api::getAllShares(array()); @@ -595,7 +595,7 @@ class Test_Files_Sharing_Api extends Test_Files_Sharing_Base { $this->assertTrue(is_string($result)); - $_GET['path'] = '/Shared'; + $_GET['path'] = '/'; $_GET['subfiles'] = 'true'; $result = Share\Api::getAllShares(array()); @@ -608,7 +608,7 @@ class Test_Files_Sharing_Api extends Test_Files_Sharing_Base { // we should get exactly one result $this->assertEquals(1, count($data)); - $expectedPath = '/Shared' . $this->filename; + $expectedPath = $this->filename; $this->assertEquals($expectedPath, $data[0]['path']); @@ -868,46 +868,4 @@ class Test_Files_Sharing_Api extends Test_Files_Sharing_Base { } - function testCorrectPath() { - $path = "/foo/bar/test.txt"; - $folder = "/correct/path"; - $expectedResult = "/correct/path/test.txt"; - - $shareApiDummy = new TestShareApi(); - - $this->assertSame($expectedResult, $shareApiDummy->correctPathTest($path, $folder)); - } - - /** - * @expectedException \Exception - */ - public function testShareNonExisting() { - \Test_Files_Sharing_Api::loginHelper(\Test_Files_Sharing_Api::TEST_FILES_SHARING_API_USER1); - - $id = PHP_INT_MAX - 1; - \OCP\Share::shareItem('file', $id, \OCP\Share::SHARE_TYPE_LINK, \Test_Files_Sharing_Api::TEST_FILES_SHARING_API_USER2, 31); - } - - /** - * @expectedException \Exception - */ - public function testShareNotOwner() { - \Test_Files_Sharing_Api::loginHelper(\Test_Files_Sharing_Api::TEST_FILES_SHARING_API_USER2); - \OC\Files\Filesystem::file_put_contents('foo.txt', 'bar'); - $info = \OC\Files\Filesystem::getFileInfo('foo.txt'); - - \Test_Files_Sharing_Api::loginHelper(\Test_Files_Sharing_Api::TEST_FILES_SHARING_API_USER1); - - \OCP\Share::shareItem('file', $info->getId(), \OCP\Share::SHARE_TYPE_LINK, \Test_Files_Sharing_Api::TEST_FILES_SHARING_API_USER2, 31); - } - -} - -/** - * @brief dumnmy class to test protected methods - */ -class TestShareApi extends \OCA\Files\Share\Api { - public function correctPathTest($path, $folder) { - return self::correctPath($path, $folder); - } } -- cgit v1.2.3 From 652d417a585ede1564456c446577aa1752253ccd Mon Sep 17 00:00:00 2001 From: Bjoern Schiessle Date: Tue, 15 Apr 2014 11:19:31 +0200 Subject: we don't allow to share a folder if it contains a share mount point --- apps/files_sharing/lib/cache.php | 7 ++-- apps/files_sharing/lib/sharedstorage.php | 5 +++ apps/files_sharing/tests/api.php | 60 ++++++++++++++++++++++++++++++++ apps/files_sharing/tests/cache.php | 6 ++-- lib/private/files/view.php | 3 +- lib/private/share/share.php | 16 +++++++++ 6 files changed, 90 insertions(+), 7 deletions(-) (limited to 'apps/files_sharing/tests/api.php') diff --git a/apps/files_sharing/lib/cache.php b/apps/files_sharing/lib/cache.php index 7e44847e404..3d9fbcf4de9 100644 --- a/apps/files_sharing/lib/cache.php +++ b/apps/files_sharing/lib/cache.php @@ -411,7 +411,7 @@ class Shared_Cache extends Cache { } /** - * get the path of a file on this storage by it's id + * get the path of a file on this storage relative to the mount point by it's id * * @param int $id * @param string $pathEnd (optional) used internally for recursive calls @@ -419,8 +419,9 @@ class Shared_Cache extends Cache { */ public function getPathById($id, $pathEnd = '') { // direct shares are easy - if ($path = $this->getShareById($id)) { - return $path . $pathEnd; + $path = $this->getShareById($id); + if (is_string($path)) { + return ltrim($pathEnd, '/'); } else { // if the item is a direct share we try and get the path of the parent and append the name of the item to it list($parent, $name) = $this->getParentInfo($id); diff --git a/apps/files_sharing/lib/sharedstorage.php b/apps/files_sharing/lib/sharedstorage.php index 25e6c0abd28..eedd279bf2b 100644 --- a/apps/files_sharing/lib/sharedstorage.php +++ b/apps/files_sharing/lib/sharedstorage.php @@ -347,6 +347,7 @@ class Shared extends \OC\Files\Storage\Common { $mountManager->addMount($mount); $mountManager->removeMount($sourcePath . '/'); $this->setUniqueName(); + $this->setMountPoint($relTargetPath); } else { \OCP\Util::writeLog('file sharing', @@ -500,6 +501,10 @@ class Shared extends \OC\Files\Storage\Common { return $this->share['share_type']; } + private function setMountPoint($path) { + $this->share['file_target'] = $path; + } + /** * @brief does the group share already has a user specific unique name * @return bool diff --git a/apps/files_sharing/tests/api.php b/apps/files_sharing/tests/api.php index 6354d1099bb..5975eb95882 100644 --- a/apps/files_sharing/tests/api.php +++ b/apps/files_sharing/tests/api.php @@ -866,6 +866,66 @@ class Test_Files_Sharing_Api extends Test_Files_Sharing_Base { $this->assertTrue($result3->succeeded()); + // cleanup + \Test_Files_Sharing_Api::loginHelper(\Test_Files_Sharing_Api::TEST_FILES_SHARING_API_USER1); + + $result = \OCP\Share::unshare('folder', $fileInfo1['fileid'], \OCP\Share::SHARE_TYPE_USER, + \Test_Files_Sharing_Api::TEST_FILES_SHARING_API_USER2); + + $this->assertTrue($result); + + + + } + + /** + * @brief share a folder which contains a share mount point, should be forbidden + */ + public function testShareFolderWithAMountPoint() { + // user 1 shares a folder with user2 + \Test_Files_Sharing_Api::loginHelper(\Test_Files_Sharing_Api::TEST_FILES_SHARING_API_USER1); + + $fileInfo = $this->view->getFileInfo($this->folder); + + $result = \OCP\Share::shareItem('folder', $fileInfo['fileid'], \OCP\Share::SHARE_TYPE_USER, + \Test_Files_Sharing_Api::TEST_FILES_SHARING_API_USER2, 31); + + $this->assertTrue($result); + + // user2 shares a file from the folder as link + \Test_Files_Sharing_Api::loginHelper(\Test_Files_Sharing_Api::TEST_FILES_SHARING_API_USER2); + + $view = new \OC\Files\View('/' . \Test_Files_Sharing_Api::TEST_FILES_SHARING_API_USER2 . '/files'); + $view->mkdir("localDir"); + + // move mount point to the folder "localDir" + $result = $view->rename($this->folder, 'localDir/'.$this->folder); + $this->assertTrue($result !== false); + + // try to share "localDir" + $fileInfo2 = $view->getFileInfo('localDir'); + + $this->assertTrue($fileInfo2 instanceof \OC\Files\FileInfo); + + try { + $result2 = \OCP\Share::shareItem('folder', $fileInfo2['fileid'], \OCP\Share::SHARE_TYPE_USER, + \Test_Files_Sharing_Api::TEST_FILES_SHARING_API_USER3, 31); + } catch (\Exception $e) { + $result2 = false; + } + + $this->assertFalse($result2); + + //cleanup + + $result = $view->rename('localDir/' . $this->folder, $this->folder); + $this->assertTrue($result !== false); + $view->unlink('localDir'); + + \Test_Files_Sharing_Api::loginHelper(\Test_Files_Sharing_Api::TEST_FILES_SHARING_API_USER1); + + \OCP\Share::unshare('folder', $fileInfo['fileid'], \OCP\Share::SHARE_TYPE_USER, + \Test_Files_Sharing_Api::TEST_FILES_SHARING_API_USER2); } } diff --git a/apps/files_sharing/tests/cache.php b/apps/files_sharing/tests/cache.php index 7a52f403d8e..b8ebeab3c39 100644 --- a/apps/files_sharing/tests/cache.php +++ b/apps/files_sharing/tests/cache.php @@ -255,7 +255,7 @@ class Test_Files_Sharing_Cache extends Test_Files_Sharing_Base { */ $sharedCache = $sharedStorage->getCache(); - $this->assertEquals('test.txt', $sharedCache->getPathById($info->getId())); + $this->assertEquals('', $sharedCache->getPathById($info->getId())); } public function testGetPathByIdShareSubFolder() { @@ -276,7 +276,7 @@ class Test_Files_Sharing_Cache extends Test_Files_Sharing_Base { */ $sharedCache = $sharedStorage->getCache(); - $this->assertEquals('foo', $sharedCache->getPathById($folderInfo->getId())); - $this->assertEquals('foo/bar/test.txt', $sharedCache->getPathById($fileInfo->getId())); + $this->assertEquals('', $sharedCache->getPathById($folderInfo->getId())); + $this->assertEquals('bar/test.txt', $sharedCache->getPathById($fileInfo->getId())); } } diff --git a/lib/private/files/view.php b/lib/private/files/view.php index 3f73632be13..a61d58aaf24 100644 --- a/lib/private/files/view.php +++ b/lib/private/files/view.php @@ -1168,7 +1168,8 @@ class View { * @var \OC\Files\Mount\Mount $mount */ $cache = $mount->getStorage()->getCache(); - if ($internalPath = $cache->getPathById($id)) { + $internalPath = $cache->getPathById($id); + if (is_string($internalPath)) { $fullPath = $mount->getMountPoint() . $internalPath; if (!is_null($path = $this->getRelativePath($fullPath))) { return $path; diff --git a/lib/private/share/share.php b/lib/private/share/share.php index 756a4d173e4..c07dd04b29f 100644 --- a/lib/private/share/share.php +++ b/lib/private/share/share.php @@ -489,6 +489,7 @@ class Share extends \OC\Share\Constants { $itemSourceName = $itemSource; } + // verify that the file exists before we try to share it if ($itemType === 'file' or $itemType === 'folder') { $path = \OC\Files\Filesystem::getPath($itemSource); @@ -499,6 +500,21 @@ class Share extends \OC\Share\Constants { } } + //verify that we don't share a folder which already contains a share mount point + if ($itemType === 'folder') { + $path = '/' . $uidOwner . '/files' . \OC\Files\Filesystem::getPath($itemSource) . '/'; + $mountManager = \OC\Files\Filesystem::getMountManager(); + $mounts = $mountManager->getAll(); + foreach ($mounts as $mountPoint => $mount) { + if ($mount->getStorage() instanceof \OC\Files\Storage\Shared && strpos($mountPoint, $path) === 0) { + $message = 'Sharing "' . $itemSourceName . '" failed, because it contains files shared with you!'; + \OC_Log::write('OCP\Share', $message, \OC_Log::ERROR); + throw new \Exception($message); + } + + } + } + // Verify share type and sharing conditions are met if ($shareType === self::SHARE_TYPE_USER) { if ($shareWith == $uidOwner) { -- cgit v1.2.3 From d468cdacf27acf1de78a7b2f07d21d1851aa8f39 Mon Sep 17 00:00:00 2001 From: Bjoern Schiessle Date: Tue, 15 Apr 2014 15:48:54 +0200 Subject: add unit tests which got lost during rebase --- apps/files_sharing/tests/api.php | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) (limited to 'apps/files_sharing/tests/api.php') diff --git a/apps/files_sharing/tests/api.php b/apps/files_sharing/tests/api.php index 5975eb95882..b2f05d10ac6 100644 --- a/apps/files_sharing/tests/api.php +++ b/apps/files_sharing/tests/api.php @@ -928,4 +928,27 @@ class Test_Files_Sharing_Api extends Test_Files_Sharing_Base { \Test_Files_Sharing_Api::TEST_FILES_SHARING_API_USER2); } + /** + * @expectedException \Exception + */ + public function testShareNonExisting() { + \Test_Files_Sharing_Api::loginHelper(\Test_Files_Sharing_Api::TEST_FILES_SHARING_API_USER1); + + $id = PHP_INT_MAX - 1; + \OCP\Share::shareItem('file', $id, \OCP\Share::SHARE_TYPE_LINK, \Test_Files_Sharing_Api::TEST_FILES_SHARING_API_USER2, 31); + } + + /** + * @expectedException \Exception + */ + public function testShareNotOwner() { + \Test_Files_Sharing_Api::loginHelper(\Test_Files_Sharing_Api::TEST_FILES_SHARING_API_USER2); + \OC\Files\Filesystem::file_put_contents('foo.txt', 'bar'); + $info = \OC\Files\Filesystem::getFileInfo('foo.txt'); + + \Test_Files_Sharing_Api::loginHelper(\Test_Files_Sharing_Api::TEST_FILES_SHARING_API_USER1); + + \OCP\Share::shareItem('file', $info->getId(), \OCP\Share::SHARE_TYPE_LINK, \Test_Files_Sharing_Api::TEST_FILES_SHARING_API_USER2, 31); + } + } -- cgit v1.2.3