From 8294ad71fcbf69960b7d6009f077ceb3c6d00168 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Thu, 2 Jul 2015 11:37:19 +0200 Subject: [PATCH 1/5] Fix the path for users which have an exception for a group share --- lib/private/share/share.php | 29 ++++++++++++++++++++++------- 1 file changed, 22 insertions(+), 7 deletions(-) diff --git a/lib/private/share/share.php b/lib/private/share/share.php index af7f78b9ff..2dc83bdcc4 100644 --- a/lib/private/share/share.php +++ b/lib/private/share/share.php @@ -142,15 +142,25 @@ class Share extends Constants { while ($source !== -1) { // Fetch all shares with another user - $query = \OC_DB::prepare( - 'SELECT `share_with`, `file_source`, `file_target` + if (!$returnUserPaths) { + $query = \OC_DB::prepare( + 'SELECT `share_with`, `file_source`, `file_target` + FROM + `*PREFIX*share` + WHERE + `item_source` = ? AND `share_type` = ? AND `item_type` IN (\'file\', \'folder\')' + ); + $result = $query->execute(array($source, self::SHARE_TYPE_USER)); + } else { + $query = \OC_DB::prepare( + 'SELECT `share_with`, `file_source`, `file_target` FROM `*PREFIX*share` WHERE - `item_source` = ? AND `share_type` = ? AND `item_type` IN (\'file\', \'folder\')' - ); - - $result = $query->execute(array($source, self::SHARE_TYPE_USER)); + `item_source` = ? AND `share_type` IN (?, ?) AND `item_type` IN (\'file\', \'folder\')' + ); + $result = $query->execute(array($source, self::SHARE_TYPE_USER, self::$shareTypeGroupUserUnique)); + } if (\OCP\DB::isError($result)) { \OCP\Util::writeLog('OCP\Share', \OC_DB::getErrorMessage(), \OC_Log::ERROR); @@ -182,7 +192,12 @@ class Share extends Constants { $shares = array_merge($shares, $usersInGroup); if ($returnUserPaths) { foreach ($usersInGroup as $user) { - $fileTargets[(int) $row['file_source']][$user] = $row; + if (!isset($fileTargets[(int) $row['file_source']][$user])) { + // When the user already has an entry for this file source + // the file is either shared directly with him as well, or + // he has an exception entry (because of naming conflict). + $fileTargets[(int) $row['file_source']][$user] = $row; + } } } } From 547c4b9a9f48d82f4b8905b13afbda5f923b25e0 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Thu, 30 Apr 2015 12:22:57 +0200 Subject: [PATCH 2/5] Add unit test for getUsersSharingFile This is to test if the user list and paths are correct, even when a recipient renamed the received shared folder --- apps/files_sharing/tests/share.php | 127 +++++++++++++++++++++++++++++ 1 file changed, 127 insertions(+) diff --git a/apps/files_sharing/tests/share.php b/apps/files_sharing/tests/share.php index 0759d0015b..ec0e21fbf8 100644 --- a/apps/files_sharing/tests/share.php +++ b/apps/files_sharing/tests/share.php @@ -321,4 +321,131 @@ class Test_Files_Sharing extends OCA\Files_sharing\Tests\TestCase { ); } + /** + * @dataProvider dataProviderGetUsersSharingFile + * + * @param string $groupName name of group to share with + * @param bool $includeOwner whether to include the owner in the result + * @param bool $includePaths whether to include paths in the result + * @param array $expectedResult expected result of the API call + */ + function testGetUsersSharingFile($groupName, $includeOwner, $includePaths, $expectedResult) { + + $fileinfo = $this->view->getFileInfo($this->folder); + + $result = \OCP\Share::shareItem('folder', $fileinfo['fileid'], \OCP\Share::SHARE_TYPE_GROUP, + $groupName, \OCP\Constants::PERMISSION_READ); + $this->assertTrue($result); + + // public share + $result = \OCP\Share::shareItem('folder', $fileinfo['fileid'], \OCP\Share::SHARE_TYPE_LINK, + null, \OCP\Constants::PERMISSION_READ); + $this->assertNotNull($result); // returns the token! + + self::loginHelper(self::TEST_FILES_SHARING_API_USER2); + + $user2View = new \OC\Files\View('/' . self::TEST_FILES_SHARING_API_USER2 . '/files'); + $user2View->rename($this->folder, $this->folder . '_renamed'); + + $ownerPath = $this->folder; + $owner = self::TEST_FILES_SHARING_API_USER1; + + $result = \OCP\Share::getUsersSharingFile($ownerPath, $owner, $includeOwner, $includePaths); + + // sort users to make sure it matches + if ($includePaths) { + ksort($result); + } else { + sort($result['users']); + } + + $this->assertEquals( + $expectedResult, + $result + ); + } + + function dataProviderGetUsersSharingFile() { + // note: "group" contains user1 (the owner), user2 and user3 + // and self::TEST_FILES_SHARING_API_GROUP1 contains only user2 + return [ + // share with group that contains owner + [ + 'group', + false, + false, + [ + 'users' => + [ + // because user1 was in group + self::TEST_FILES_SHARING_API_USER1, + self::TEST_FILES_SHARING_API_USER2, + self::TEST_FILES_SHARING_API_USER3, + ], + 'public' => true, + 'remote' => false, + ], + ], + // share with group that does not contain owner + [ + self::TEST_FILES_SHARING_API_GROUP1, + false, + false, + [ + 'users' => + [ + self::TEST_FILES_SHARING_API_USER2, + ], + 'public' => true, + 'remote' => false, + ], + ], + // share with group that does not contain owner, include owner + [ + self::TEST_FILES_SHARING_API_GROUP1, + true, + false, + [ + 'users' => + [ + self::TEST_FILES_SHARING_API_USER1, + self::TEST_FILES_SHARING_API_USER2, + ], + 'public' => true, + 'remote' => false, + ], + ], + // include paths, with owner + [ + 'group', + true, + true, + [ + self::TEST_FILES_SHARING_API_USER1 => self::TEST_FOLDER_NAME, + self::TEST_FILES_SHARING_API_USER2 => self::TEST_FOLDER_NAME. '_renamed', + self::TEST_FILES_SHARING_API_USER3 => self::TEST_FOLDER_NAME, + ], + ], + // include paths, group without owner + [ + self::TEST_FILES_SHARING_API_GROUP1, + false, + true, + [ + self::TEST_FILES_SHARING_API_USER2 => self::TEST_FOLDER_NAME. '_renamed', + ], + ], + // include paths, include owner, group without owner + [ + self::TEST_FILES_SHARING_API_GROUP1, + true, + true, + [ + self::TEST_FILES_SHARING_API_USER1 => self::TEST_FOLDER_NAME, + self::TEST_FILES_SHARING_API_USER2 => self::TEST_FOLDER_NAME. '_renamed', + ], + ], + ]; + } + } From fb0fef78f46563fad4400d18a15a270fb4b47c24 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Thu, 30 Apr 2015 13:02:04 +0200 Subject: [PATCH 3/5] Add test case when owner renames shared folder --- apps/files_sharing/tests/share.php | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/apps/files_sharing/tests/share.php b/apps/files_sharing/tests/share.php index ec0e21fbf8..a34f4f4e63 100644 --- a/apps/files_sharing/tests/share.php +++ b/apps/files_sharing/tests/share.php @@ -342,12 +342,15 @@ class Test_Files_Sharing extends OCA\Files_sharing\Tests\TestCase { null, \OCP\Constants::PERMISSION_READ); $this->assertNotNull($result); // returns the token! + // owner renames after sharing + $this->view->rename($this->folder, $this->folder . '_owner_renamed'); + self::loginHelper(self::TEST_FILES_SHARING_API_USER2); $user2View = new \OC\Files\View('/' . self::TEST_FILES_SHARING_API_USER2 . '/files'); $user2View->rename($this->folder, $this->folder . '_renamed'); - $ownerPath = $this->folder; + $ownerPath = $this->folder . '_owner_renamed'; $owner = self::TEST_FILES_SHARING_API_USER1; $result = \OCP\Share::getUsersSharingFile($ownerPath, $owner, $includeOwner, $includePaths); @@ -421,8 +424,8 @@ class Test_Files_Sharing extends OCA\Files_sharing\Tests\TestCase { true, true, [ - self::TEST_FILES_SHARING_API_USER1 => self::TEST_FOLDER_NAME, - self::TEST_FILES_SHARING_API_USER2 => self::TEST_FOLDER_NAME. '_renamed', + self::TEST_FILES_SHARING_API_USER1 => self::TEST_FOLDER_NAME . '_owner_renamed', + self::TEST_FILES_SHARING_API_USER2 => self::TEST_FOLDER_NAME . '_renamed', self::TEST_FILES_SHARING_API_USER3 => self::TEST_FOLDER_NAME, ], ], @@ -441,8 +444,8 @@ class Test_Files_Sharing extends OCA\Files_sharing\Tests\TestCase { true, true, [ - self::TEST_FILES_SHARING_API_USER1 => self::TEST_FOLDER_NAME, - self::TEST_FILES_SHARING_API_USER2 => self::TEST_FOLDER_NAME. '_renamed', + self::TEST_FILES_SHARING_API_USER1 => self::TEST_FOLDER_NAME . '_owner_renamed', + self::TEST_FILES_SHARING_API_USER2 => self::TEST_FOLDER_NAME . '_renamed', ], ], ]; From 594f5b6a29c542f2563db91a4c1c6ce6287acd22 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Tue, 7 Jul 2015 11:07:11 +0200 Subject: [PATCH 4/5] Add visibility to test methods --- apps/files_sharing/tests/share.php | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/apps/files_sharing/tests/share.php b/apps/files_sharing/tests/share.php index a34f4f4e63..ed21e20f75 100644 --- a/apps/files_sharing/tests/share.php +++ b/apps/files_sharing/tests/share.php @@ -65,7 +65,7 @@ class Test_Files_Sharing extends OCA\Files_sharing\Tests\TestCase { parent::tearDown(); } - function testUnshareFromSelf() { + public function testUnshareFromSelf() { \OC_Group::createGroup('testGroup'); \OC_Group::addToGroup(self::TEST_FILES_SHARING_API_USER2, 'testGroup'); @@ -103,7 +103,7 @@ class Test_Files_Sharing extends OCA\Files_sharing\Tests\TestCase { /** * if a file was shared as group share and as individual share they should be grouped */ - function testGroupingOfShares() { + public function testGroupingOfShares() { $fileinfo = $this->view->getFileInfo($this->filename); @@ -171,7 +171,7 @@ class Test_Files_Sharing extends OCA\Files_sharing\Tests\TestCase { * single user share to user2. If he re-shares the file to user2 the same target * then the group share should be used to group the item */ - function testShareAndUnshareFromSelf() { + public function testShareAndUnshareFromSelf() { $fileinfo = $this->view->getFileInfo($this->filename); // share the file to group1 (user2 is a member of this group) and explicitely to user2 @@ -217,7 +217,7 @@ class Test_Files_Sharing extends OCA\Files_sharing\Tests\TestCase { } - function verifyDirContent($content, $expected) { + public function verifyDirContent($content, $expected) { foreach ($content as $c) { if (!in_array($c['name'], $expected)) { $this->assertTrue(false, "folder should only contain '" . implode(',', $expected) . "', found: " .$c['name']); @@ -225,7 +225,7 @@ class Test_Files_Sharing extends OCA\Files_sharing\Tests\TestCase { } } - function testShareWithDifferentShareFolder() { + public function testShareWithDifferentShareFolder() { $fileinfo = $this->view->getFileInfo($this->filename); $folderinfo = $this->view->getFileInfo($this->folder); @@ -249,7 +249,7 @@ class Test_Files_Sharing extends OCA\Files_sharing\Tests\TestCase { \OC::$server->getConfig()->deleteSystemValue('share_folder'); } - function testShareWithGroupUniqueName() { + public function testShareWithGroupUniqueName() { $this->loginHelper(self::TEST_FILES_SHARING_API_USER1); \OC\Files\Filesystem::file_put_contents('test.txt', 'test'); @@ -283,9 +283,9 @@ class Test_Files_Sharing extends OCA\Files_sharing\Tests\TestCase { /** * shared files should never have delete permissions - * @dataProvider DataProviderTestFileSharePermissions + * @dataProvider dataProviderTestFileSharePermissions */ - function testFileSharePermissions($permission, $expectedPermissions) { + public function testFileSharePermissions($permission, $expectedPermissions) { $fileinfo = $this->view->getFileInfo($this->filename); @@ -305,7 +305,7 @@ class Test_Files_Sharing extends OCA\Files_sharing\Tests\TestCase { $this->assertSame($expectedPermissions, $share['permissions']); } - function DataProviderTestFileSharePermissions() { + public function dataProviderTestFileSharePermissions() { $permission1 = \OCP\Constants::PERMISSION_ALL; $permission3 = \OCP\Constants::PERMISSION_READ; $permission4 = \OCP\Constants::PERMISSION_READ | \OCP\Constants::PERMISSION_UPDATE; @@ -329,7 +329,7 @@ class Test_Files_Sharing extends OCA\Files_sharing\Tests\TestCase { * @param bool $includePaths whether to include paths in the result * @param array $expectedResult expected result of the API call */ - function testGetUsersSharingFile($groupName, $includeOwner, $includePaths, $expectedResult) { + public function testGetUsersSharingFile($groupName, $includeOwner, $includePaths, $expectedResult) { $fileinfo = $this->view->getFileInfo($this->folder); @@ -368,7 +368,7 @@ class Test_Files_Sharing extends OCA\Files_sharing\Tests\TestCase { ); } - function dataProviderGetUsersSharingFile() { + public function dataProviderGetUsersSharingFile() { // note: "group" contains user1 (the owner), user2 and user3 // and self::TEST_FILES_SHARING_API_GROUP1 contains only user2 return [ From 40eaf71a333267288873462a33855bd0d082f21d Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Tue, 7 Jul 2015 11:08:21 +0200 Subject: [PATCH 5/5] Make sure the owner always has the right path --- lib/private/share/share.php | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/lib/private/share/share.php b/lib/private/share/share.php index 2dc83bdcc4..2a9fa44f78 100644 --- a/lib/private/share/share.php +++ b/lib/private/share/share.php @@ -253,9 +253,6 @@ class Share extends Constants { // Include owner in list of users, if requested if ($includeOwner) { $shares[] = $ownerUser; - if ($returnUserPaths) { - $sharePaths[$ownerUser] = $path; - } } if ($returnUserPaths) { @@ -283,6 +280,12 @@ class Share extends Constants { } } + if ($includeOwner) { + $sharePaths[$ownerUser] = $path; + } else { + unset($sharePaths[$ownerUser]); + } + return $sharePaths; }