From 64a9315b4a89d94eab45b996d740600c13476734 Mon Sep 17 00:00:00 2001 From: Bjoern Schiessle Date: Thu, 31 Jul 2014 11:55:59 +0200 Subject: group shares and combine permissions --- apps/files_sharing/lib/share/file.php | 2 +- apps/files_sharing/lib/sharedmount.php | 15 +- apps/files_sharing/lib/sharedstorage.php | 15 + lib/private/share/share.php | 484 ++++++++++++++++--------------- 4 files changed, 276 insertions(+), 240 deletions(-) diff --git a/apps/files_sharing/lib/share/file.php b/apps/files_sharing/lib/share/file.php index 2ae7fdc16ab..0cd66547d0b 100644 --- a/apps/files_sharing/lib/share/file.php +++ b/apps/files_sharing/lib/share/file.php @@ -38,7 +38,7 @@ class OC_Share_Backend_File implements OCP\Share_Backend_File_Dependent { // FIXME: attributes should not be set here, // keeping this pattern for now to avoid unexpected // regressions - $this->path = basename($path); + $this->path = \OC\Files\Filesystem::normalizePath(basename($path)); return true; } return false; diff --git a/apps/files_sharing/lib/sharedmount.php b/apps/files_sharing/lib/sharedmount.php index 564ac43ec74..9469e0aa48d 100644 --- a/apps/files_sharing/lib/sharedmount.php +++ b/apps/files_sharing/lib/sharedmount.php @@ -121,7 +121,15 @@ class SharedMount extends Mount implements MoveableMount { $relTargetPath = $this->stripUserFilesPath($target); $share = $this->storage->getShare(); - $result = $this->updateFileTarget($relTargetPath, $share); + $result = true; + + if (!empty($share['grouped'])) { + foreach ($share['grouped'] as $s) { + $result = $this->updateFileTarget($relTargetPath, $s) && $result; + } + } else { + $result = $this->updateFileTarget($relTargetPath, $share) && $result; + } if ($result) { $this->setMountPoint($target); @@ -144,8 +152,11 @@ class SharedMount extends Mount implements MoveableMount { */ public function removeMount() { $mountManager = \OC\Files\Filesystem::getMountManager(); + /** + * @var \OC\Files\Storage\Shared + */ $storage = $this->getStorage(); - $result = \OCP\Share::unshareFromSelf($storage->getItemType(), $storage->getMountPoint()); + $result = $storage->unshareStorage(); $mountManager->removeMount($this->mountPoint); return $result; diff --git a/apps/files_sharing/lib/sharedstorage.php b/apps/files_sharing/lib/sharedstorage.php index 02fcd7041dd..0bd7c6f4fd5 100644 --- a/apps/files_sharing/lib/sharedstorage.php +++ b/apps/files_sharing/lib/sharedstorage.php @@ -532,4 +532,19 @@ class Shared extends \OC\Files\Storage\Common implements ISharedStorage { return null; } + /** + * unshare complete storage, also the grouped shares + */ + public function unshareStorage() { + $result = true; + if (!empty($this->share['grouped'])) { + foreach ($this->share['grouped'] as $share) { + $result = $result && \OCP\Share::unshareFromSelf($share['item_type'], $share['file_target']); + } + } + $result = $result && \OCP\Share::unshareFromSelf($this->getItemType(), $this->getMountPoint()); + + return $result; + } + } diff --git a/lib/private/share/share.php b/lib/private/share/share.php index 22e354e515d..2864e8532aa 100644 --- a/lib/private/share/share.php +++ b/lib/private/share/share.php @@ -339,11 +339,13 @@ class Share extends \OC\Share\Constants { * @param int $format (optional) Format type must be defined by the backend * @param mixed $parameters * @param boolean $includeCollections + * @param string $shareWith (optional) define against which user should be checked, default: current user * @return mixed Return depends on format */ public static function getItemSharedWithBySource($itemType, $itemSource, $format = self::FORMAT_NONE, - $parameters = null, $includeCollections = false) { - return self::getItems($itemType, $itemSource, self::$shareTypeUserAndGroups, \OC_User::getUser(), null, $format, + $parameters = null, $includeCollections = false, $shareWith = null) { + $shareWith = ($shareWith === null) ? \OC_User::getUser() : $shareWith; + return self::getItems($itemType, $itemSource, self::$shareTypeUserAndGroups, $shareWith, null, $format, $parameters, 1, $includeCollections, true); } @@ -1293,7 +1295,7 @@ class Share extends \OC\Share\Constants { // Make sure the unique user target is returned if it exists, // unique targets should follow the group share in the database // If the limit is not 1, the filtering can be done later - $where .= ' ORDER BY `*PREFIX*share`.`id` DESC'; + $where .= ' ORDER BY `*PREFIX*share`.`id` ASC'; } // The limit must be at least 3, because filtering needs to be done if ($limit < 3) { @@ -1311,7 +1313,7 @@ class Share extends \OC\Share\Constants { $result = $query->execute($queryArgs); if (\OC_DB::isError($result)) { \OC_Log::write('OCP\Share', - \OC_DB::getErrorMessage($result) . ', select=' . $select . ' where=' . $where, + \OC_DB::getErrorMessage($result) . ', select=' . $select . ' where=', \OC_Log::ERROR); } $items = array(); @@ -1412,7 +1414,13 @@ class Share extends \OC\Share\Constants { } $items[$row['id']] = $row; + + } + + if (isset($shareWith) && $shareWith === \OCP\User::getUser()) { + $items = self::groupItems($items, $itemType); } + if (!empty($items)) { $collectionItems = array(); foreach ($items as &$row) { @@ -1498,6 +1506,47 @@ class Share extends \OC\Share\Constants { } /** + * group items with link to the same source + * + * @param array $items + * @param string $itemType + * @return array of grouped items + */ + private static function groupItems($items, $itemType) { + + $fileSharing = ($itemType === 'file' || $itemType === 'folder') ? true : false; + + $result = array(); + + foreach ($items as $item) { + $grouped = false; + foreach ($result as $key => $r) { + // for file/folder shares we need to compare file_source, otherwise we compare item_source + // only group shares if they already point to the same target, otherwise the file where shared + // before grouping of shares was added. In this case we don't group them toi avoid confusions + if (( $fileSharing && $item['file_source'] === $r['file_source'] && $item['file_target'] === $r['file_target']) || + (!$fileSharing && $item['item_source'] === $r['item_source'] && $item['item_target'] === $r['item_target'])) { + // add the first item to the list of grouped shares + if (!isset($result[$key]['grouped'])) { + $result[$key]['grouped'][] = $result[$key]; + } + $result[$key]['permissions'] = (int) $item['permissions'] | (int) $r['permissions']; + $result[$key]['grouped'][] = $item; + $grouped = true; + break; + } + } + + if (!$grouped) { + $result[] = $item; + } + + } + + return $result; + } + +/** * Put shared item into the database * @param string $itemType Item type * @param string $itemSource Item source @@ -1514,11 +1563,182 @@ class Share extends \OC\Share\Constants { */ private static function put($itemType, $itemSource, $shareType, $shareWith, $uidOwner, $permissions, $parentFolder = null, $token = null, $itemSourceName = null, \DateTime $expirationDate = null) { + + $queriesToExecute = array(); + + $fileShare = false; + if ($itemType === 'file' || $itemType === 'folder') { + $fileShare = true; + } + + $result = self::checkReshare($itemType, $itemSource, $shareType, $shareWith, $uidOwner, $permissions, $itemSourceName, $expirationDate); + if(!empty($result)) { + $parent = $result['parent']; + $itemSource = $result['itemSource']; + $fileSource = $result['fileSource']; + $suggestedItemTarget = $result['suggestedItemTarget']; + $suggestedFileTarget = $result['suggestedFileTarget']; + $filePath = $result['filePath']; + $expirationDate = $result['expirationDate']; + } + + $isGroupShare = false; + if ($shareType == self::SHARE_TYPE_GROUP) { + $isGroupShare = true; + $users = \OC_Group::usersInGroup($shareWith['group']); + // remove current user from list + if (in_array(\OCP\User::getUser(), $users)) { + unset($users[array_search(\OCP\User::getUser(), $users)]); + } + $groupItemTarget = Helper::generateTarget($itemType, $itemSource, $shareType, $shareWith['group'], + $uidOwner, $suggestedItemTarget); + $groupFileTarget = $filePath; + + // add group share to table and remember the id as parent + $queriesToExecute['groupShare'] = array( + 'itemType' => $itemType, + 'itemSource' => $itemSource, + 'itemTarget' => $groupItemTarget, + 'shareType' => $shareType, + 'shareWith' => $shareWith['group'], + 'uidOwner' => $uidOwner, + 'permissions' => $permissions, + 'shareTime' => time(), + 'fileSource' => $fileSource, + 'fileTarget' => $filePath, + 'token' => $token, + 'parent' => $parent, + 'expiration' => $expirationDate, + ); + + } else { + $users = array($shareWith); + $itemTarget = Helper::generateTarget($itemType, $itemSource, $shareType, $shareWith, $uidOwner, + $suggestedItemTarget); + } + + $run = true; + $error = ''; + $preHookData = array( + 'itemType' => $itemType, + 'itemSource' => $itemSource, + 'shareType' => $shareType, + 'uidOwner' => $uidOwner, + 'permissions' => $permissions, + 'fileSource' => $fileSource, + 'expiration' => $expirationDate, + 'token' => $token, + 'run' => &$run, + 'error' => &$error + ); + + $preHookData['itemTarget'] = ($isGroupShare) ? $groupItemTarget : $itemTarget; + $preHookData['shareWith'] = ($isGroupShare) ? $shareWith['group'] : $shareWith; + + \OC_Hook::emit('OCP\Share', 'pre_shared', $preHookData); + + if ($run === false) { + throw new \Exception($error); + } + + foreach ($users as $user) { + $sourceExists = self::getItemSharedWithBySource($itemType, $itemSource, self::FORMAT_NONE, null, true, $user); + + $shareType = ($isGroupShare) ? self::$shareTypeGroupUserUnique : $shareType; + + if ($sourceExists) { + $fileTarget = $sourceExists['file_target']; + $itemTarget = $sourceExists['item_target']; + + } elseif(!$sourceExists && !$isGroupShare) { + + $itemTarget = Helper::generateTarget($itemType, $itemSource, self::SHARE_TYPE_USER, $user, + $uidOwner, $suggestedItemTarget, $parent); + if (isset($fileSource)) { + if ($parentFolder) { + if ($parentFolder === true) { + $fileTarget = Helper::generateTarget('file', $filePath, self::SHARE_TYPE_USER, $user, + $uidOwner, $suggestedFileTarget, $parent); + if ($fileTarget != $groupFileTarget) { + $parentFolders[$user]['folder'] = $fileTarget; + } + } else if (isset($parentFolder[$user])) { + $fileTarget = $parentFolder[$user]['folder'].$itemSource; + $parent = $parentFolder[$user]['id']; + } + } else { + $fileTarget = Helper::generateTarget('file', $filePath, self::SHARE_TYPE_USER, + $user, $uidOwner, $suggestedFileTarget, $parent); + } + } else { + $fileTarget = null; + } + + } else { + // move to the next user if it is neither a unique group share + // nor a single share without adding it to the share table + continue; + } + + $queriesToExecute[] = array( + 'itemType' => $itemType, + 'itemSource' => $itemSource, + 'itemTarget' => $itemTarget, + 'shareType' => $shareType, + 'shareWith' => $user, + 'uidOwner' => $uidOwner, + 'permissions' => $permissions, + 'shareTime' => time(), + 'fileSource' => $fileSource, + 'fileTarget' => $fileTarget, + 'token' => $token, + 'parent' => $parent, + 'expiration' => $expirationDate, + ); + + } + + if ($isGroupShare) { + self::insertShare($queriesToExecute['groupShare']); + // Save this id, any extra rows for this group share will need to reference it + $parent = \OC_DB::insertid('*PREFIX*share'); + unset($queriesToExecute['groupShare']); + } + + foreach ($queriesToExecute as $shareQuery) { + $shareQuery['parent'] = $parent; + self::insertShare($shareQuery); + } + + $postHookData = array( + 'itemType' => $itemType, + 'itemSource' => $itemSource, + 'parent' => $parent, + 'shareType' => $shareType, + 'uidOwner' => $uidOwner, + 'permissions' => $permissions, + 'fileSource' => $fileSource, + 'id' => $parent, + 'token' => $token, + 'expirationDate' => $expirationDate, + ); + + $postHookData['shareWith'] = ($isGroupShare) ? $shareWith['group'] : $shareWith; + $postHookData['itemTarget'] = ($isGroupShare) ? $groupItemTarget : $itemTarget; + $postHookData['fileTarget'] = ($isGroupShare) ? $groupFileTarget : $fileTarget; + + \OC_Hook::emit('OCP\Share', 'post_shared', $postHookData); + + + return true; + } + + private static function checkReshare($itemType, $itemSource, $shareType, $shareWith, $uidOwner, $permissions, $itemSourceName, $expirationDate) { $backend = self::getBackend($itemType); $l = \OC_L10N::get('lib'); - // Check if this is a reshare - if ($checkReshare = self::getItemSharedWithBySource($itemType, $itemSource, self::FORMAT_NONE, null, true)) { + $checkReshare = self::getItemSharedWithBySource($itemType, $itemSource, self::FORMAT_NONE, null, true); + if ($checkReshare) { // Check if attempting to share back to owner if ($checkReshare['uid_owner'] == $shareWith && $shareType == self::SHARE_TYPE_USER) { $message = 'Sharing %s failed, because the user %s is the original sharer'; @@ -1527,6 +1747,7 @@ class Share extends \OC\Share\Constants { \OC_Log::write('OCP\Share', sprintf($message, $itemSourceName, $shareWith), \OC_Log::ERROR); throw new \Exception($message_t); } + // Check if share permissions is granted if (self::isResharingAllowed() && (int)$checkReshare['permissions'] & \OCP\PERMISSION_SHARE) { if (~(int)$checkReshare['permissions'] & $permissions) { @@ -1537,13 +1758,13 @@ class Share extends \OC\Share\Constants { throw new \Exception($message_t); } else { // TODO Don't check if inside folder - $parent = $checkReshare['id']; - $itemSource = $checkReshare['item_source']; - $fileSource = $checkReshare['file_source']; - $suggestedItemTarget = $checkReshare['item_target']; - $suggestedFileTarget = $checkReshare['file_target']; - $filePath = $checkReshare['file_target']; - $expirationDate = min($expirationDate, $checkReshare['expiration']); + $result['parent'] = $checkReshare['id']; + $result['itemSource'] = $checkReshare['item_source']; + $result['fileSource'] = $checkReshare['file_source']; + $result['suggestedItemTarget'] = $checkReshare['item_target']; + $result['suggestedFileTarget'] = $checkReshare['file_target']; + $result['filePath'] = $checkReshare['file_target']; + $result['expirationDate'] = min($expirationDate, $checkReshare['expiration']); } } else { $message = 'Sharing %s failed, because resharing is not allowed'; @@ -1553,9 +1774,11 @@ class Share extends \OC\Share\Constants { throw new \Exception($message_t); } } else { - $parent = null; - $suggestedItemTarget = null; - $suggestedFileTarget = null; + $result['parent'] = null; + $result['suggestedItemTarget'] = null; + $result['suggestedFileTarget'] = null; + $result['itemSource'] = $itemSource; + $result['expirationDate'] = $expirationDate; if (!$backend->isValidSource($itemSource, $uidOwner)) { $message = 'Sharing %s failed, because the sharing backend for ' .'%s could not find its source'; @@ -1564,14 +1787,14 @@ class Share extends \OC\Share\Constants { throw new \Exception($message_t); } if ($backend instanceof \OCP\Share_Backend_File_Dependent) { - $filePath = $backend->getFilePath($itemSource, $uidOwner); + $result['filePath'] = $backend->getFilePath($itemSource, $uidOwner); if ($itemType == 'file' || $itemType == 'folder') { - $fileSource = $itemSource; + $result['fileSource'] = $itemSource; } else { - $meta = \OC\Files\Filesystem::getFileInfo($filePath); - $fileSource = $meta['fileid']; + $meta = \OC\Files\Filesystem::getFileInfo($result['filePath']); + $result['fileSource'] = $meta['fileid']; } - if ($fileSource == -1) { + if ($result['fileSource'] == -1) { $message = 'Sharing %s failed, because the file could not be found in the file cache'; $message_t = $l->t('Sharing %s failed, because the file could not be found in the file cache', array($itemSource)); @@ -1579,225 +1802,12 @@ class Share extends \OC\Share\Constants { throw new \Exception($message_t); } } else { - $filePath = null; - $fileSource = null; + $result['filePath'] = null; + $result['fileSource'] = null; } } - // Share with a group - if ($shareType == self::SHARE_TYPE_GROUP) { - $groupItemTarget = Helper::generateTarget($itemType, $itemSource, $shareType, $shareWith['group'], - $uidOwner, $suggestedItemTarget); - $run = true; - $error = ''; - \OC_Hook::emit('OCP\Share', 'pre_shared', array( - 'itemType' => $itemType, - 'itemSource' => $itemSource, - 'itemTarget' => $groupItemTarget, - 'shareType' => $shareType, - 'shareWith' => $shareWith['group'], - 'uidOwner' => $uidOwner, - 'permissions' => $permissions, - 'fileSource' => $fileSource, - 'expiration' => $expirationDate, - 'token' => $token, - 'run' => &$run, - 'error' => &$error - )); - - if ($run === false) { - throw new \Exception($error); - } - - if (isset($fileSource)) { - if ($parentFolder) { - if ($parentFolder === true) { - $groupFileTarget = Helper::generateTarget('file', $filePath, $shareType, - $shareWith['group'], $uidOwner, $suggestedFileTarget); - // Set group default file target for future use - $parentFolders[0]['folder'] = $groupFileTarget; - } else { - // Get group default file target - $groupFileTarget = $parentFolder[0]['folder'].$itemSource; - $parent = $parentFolder[0]['id']; - } - } else { - $groupFileTarget = Helper::generateTarget('file', $filePath, $shareType, $shareWith['group'], - $uidOwner, $suggestedFileTarget); - } - } else { - $groupFileTarget = null; - } - $queriesToExecute = array(); - $queriesToExecute['groupShare'] = array( - 'itemType' => $itemType, - 'itemSource' => $itemSource, - 'itemTarget' => $groupItemTarget, - 'shareType' => $shareType, - 'shareWith' => $shareWith['group'], - 'uidOwner' => $uidOwner, - 'permissions' => $permissions, - 'shareTime' => time(), - 'fileSource' => $fileSource, - 'fileTarget' => $groupFileTarget, - 'token' => $token, - 'parent' => $parent, - 'expiration' => $expirationDate, - ); - // Loop through all users of this group in case we need to add an extra row - foreach ($shareWith['users'] as $uid) { - $itemTarget = Helper::generateTarget($itemType, $itemSource, self::SHARE_TYPE_USER, $uid, - $uidOwner, $suggestedItemTarget, $parent); - if (isset($fileSource)) { - if ($parentFolder) { - if ($parentFolder === true) { - $fileTarget = Helper::generateTarget('file', $filePath, self::SHARE_TYPE_USER, $uid, - $uidOwner, $suggestedFileTarget, $parent); - if ($fileTarget != $groupFileTarget) { - $parentFolders[$uid]['folder'] = $fileTarget; - } - } else if (isset($parentFolder[$uid])) { - $fileTarget = $parentFolder[$uid]['folder'].$itemSource; - $parent = $parentFolder[$uid]['id']; - } - } else { - $fileTarget = Helper::generateTarget('file', $filePath, self::SHARE_TYPE_USER, - $uid, $uidOwner, $suggestedFileTarget, $parent); - } - } else { - $fileTarget = null; - } - // Insert an extra row for the group share if the item or file target is unique for this user - if ($itemTarget != $groupItemTarget || (isset($fileSource) && $fileTarget != $groupFileTarget)) { - $queriesToExecute[] = array( - 'itemType' => $itemType, - 'itemSource' => $itemSource, - 'itemTarget' => $itemTarget, - 'shareType' => self::$shareTypeGroupUserUnique, - 'shareWith' => $uid, - 'uidOwner' => $uidOwner, - 'permissions' => $permissions, - 'shareTime' => time(), - 'fileSource' => $fileSource, - 'fileTarget' => $fileTarget, - 'token' => $token, - //'parent' => $parent, - 'expiration' => $expirationDate, - ); - } - } - - self::insertShare($queriesToExecute['groupShare']); - // Save this id, any extra rows for this group share will need to reference it - $parent = \OC_DB::insertid('*PREFIX*share'); - unset($queriesToExecute['groupShare']); - - foreach ($queriesToExecute as $shareQuery) { - $shareQuery['parent'] = $parent; - self::insertShare($shareQuery); - } - - \OC_Hook::emit('OCP\Share', 'post_shared', array( - 'itemType' => $itemType, - 'itemSource' => $itemSource, - 'itemTarget' => $groupItemTarget, - 'parent' => $parent, - 'shareType' => $shareType, - 'shareWith' => $shareWith['group'], - 'uidOwner' => $uidOwner, - 'permissions' => $permissions, - 'fileSource' => $fileSource, - 'fileTarget' => $groupFileTarget, - 'id' => $parent, - 'token' => $token, - 'expirationDate' => $expirationDate, - )); - - if ($parentFolder === true) { - // Return parent folders to preserve file target paths for potential children - return $parentFolders; - } - } else { - $itemTarget = Helper::generateTarget($itemType, $itemSource, $shareType, $shareWith, $uidOwner, - $suggestedItemTarget); - $run = true; - $error = ''; - \OC_Hook::emit('OCP\Share', 'pre_shared', array( - 'itemType' => $itemType, - 'itemSource' => $itemSource, - 'itemTarget' => $itemTarget, - 'shareType' => $shareType, - 'shareWith' => $shareWith, - 'uidOwner' => $uidOwner, - 'permissions' => $permissions, - 'fileSource' => $fileSource, - 'token' => $token, - 'expirationDate' => $expirationDate, - 'run' => &$run, - 'error' => &$error, - )); - - if ($run === false) { - throw new \Exception($error); - } - - if (isset($fileSource)) { - if ($parentFolder) { - if ($parentFolder === true) { - $fileTarget = Helper::generateTarget('file', $filePath, $shareType, $shareWith, - $uidOwner, $suggestedFileTarget); - $parentFolders['folder'] = $fileTarget; - } else { - $fileTarget = $parentFolder['folder'].$itemSource; - $parent = $parentFolder['id']; - } - } else { - $fileTarget = Helper::generateTarget('file', $filePath, $shareType, $shareWith, $uidOwner, - $suggestedFileTarget); - } - } else { - $fileTarget = null; - } - - self::insertShare(array( - 'itemType' => $itemType, - 'itemSource' => $itemSource, - 'itemTarget' => $itemTarget, - 'shareType' => $shareType, - 'shareWith' => $shareWith, - 'uidOwner' => $uidOwner, - 'permissions' => $permissions, - 'shareTime' => time(), - 'fileSource' => $fileSource, - 'fileTarget' => $fileTarget, - 'token' => $token, - 'parent' => $parent, - 'expiration' => $expirationDate, - )); - - $id = \OC_DB::insertid('*PREFIX*share'); - \OC_Hook::emit('OCP\Share', 'post_shared', array( - 'itemType' => $itemType, - 'itemSource' => $itemSource, - 'itemTarget' => $itemTarget, - 'parent' => $parent, - 'shareType' => $shareType, - 'shareWith' => $shareWith, - 'uidOwner' => $uidOwner, - 'permissions' => $permissions, - 'fileSource' => $fileSource, - 'fileTarget' => $fileTarget, - 'id' => $id, - 'token' => $token, - 'expirationDate' => $expirationDate, - )); - if ($parentFolder === true) { - $parentFolders['id'] = $id; - // Return parent folder to preserve file target paths for potential children - return $parentFolders; - } - } - return true; + return $result; } private static function insertShare(array $shareData) -- cgit v1.2.3 From b52154b306f13327a593fdc1231fcd454720e0a6 Mon Sep 17 00:00:00 2001 From: Bjoern Schiessle Date: Fri, 1 Aug 2014 16:24:19 +0200 Subject: unit tests for grouping of shares pointing to the same source --- apps/files_sharing/tests/base.php | 9 ++++ apps/files_sharing/tests/share.php | 100 ++++++++++++++++++++++++------------- lib/private/share/share.php | 2 +- tests/lib/share/share.php | 92 +++++++++++++++++++++++++++++----- 4 files changed, 155 insertions(+), 48 deletions(-) diff --git a/apps/files_sharing/tests/base.php b/apps/files_sharing/tests/base.php index 34ec4a36ede..60159d84e7e 100644 --- a/apps/files_sharing/tests/base.php +++ b/apps/files_sharing/tests/base.php @@ -35,6 +35,8 @@ abstract class Test_Files_Sharing_Base extends \PHPUnit_Framework_TestCase { const TEST_FILES_SHARING_API_USER2 = "test-share-user2"; const TEST_FILES_SHARING_API_USER3 = "test-share-user3"; + const TEST_FILES_SHARING_API_GROUP1 = "test-share-group1"; + public $stateFilesEncryption; public $filename; public $data; @@ -60,6 +62,10 @@ abstract class Test_Files_Sharing_Base extends \PHPUnit_Framework_TestCase { self::loginHelper(self::TEST_FILES_SHARING_API_USER2, true); self::loginHelper(self::TEST_FILES_SHARING_API_USER3, true); + // create group + \OC_Group::createGroup(self::TEST_FILES_SHARING_API_GROUP1); + \OC_Group::addToGroup(self::TEST_FILES_SHARING_API_USER2, self::TEST_FILES_SHARING_API_GROUP1); + } function setUp() { @@ -94,6 +100,9 @@ abstract class Test_Files_Sharing_Base extends \PHPUnit_Framework_TestCase { \OC_User::deleteUser(self::TEST_FILES_SHARING_API_USER1); \OC_User::deleteUser(self::TEST_FILES_SHARING_API_USER2); \OC_User::deleteUser(self::TEST_FILES_SHARING_API_USER3); + + // delete group + \OC_Group::deleteGroup(self::TEST_FILES_SHARING_API_GROUP1); } /** diff --git a/apps/files_sharing/tests/share.php b/apps/files_sharing/tests/share.php index 31246c5df44..c780a9e816a 100644 --- a/apps/files_sharing/tests/share.php +++ b/apps/files_sharing/tests/share.php @@ -57,6 +57,10 @@ class Test_Files_Sharing extends Test_Files_Sharing_Base { self::$tempStorage = null; + // clear database table + $query = \OCP\DB::prepare('DELETE FROM `*PREFIX*share`'); + $query->execute(); + parent::tearDown(); } @@ -70,8 +74,6 @@ class Test_Files_Sharing extends Test_Files_Sharing_Base { $pathinfo = pathinfo($this->filename); - $duplicate = '/' . $pathinfo['filename'] . ' (2).' . $pathinfo['extension']; - $result = \OCP\Share::shareItem('file', $fileinfo['fileid'], \OCP\Share::SHARE_TYPE_USER, \Test_Files_Sharing::TEST_FILES_SHARING_API_USER2, 31); @@ -84,46 +86,85 @@ class Test_Files_Sharing extends Test_Files_Sharing_Base { self::loginHelper(self::TEST_FILES_SHARING_API_USER2); $this->assertTrue(\OC\Files\Filesystem::file_exists($this->filename)); - $this->assertTrue(\OC\Files\Filesystem::file_exists($duplicate)); self::loginHelper(self::TEST_FILES_SHARING_API_USER3); $this->assertTrue(\OC\Files\Filesystem::file_exists($this->filename)); - $this->assertFalse(\OC\Files\Filesystem::file_exists($duplicate)); self::loginHelper(self::TEST_FILES_SHARING_API_USER2); \OC\Files\Filesystem::unlink($this->filename); self::loginHelper(self::TEST_FILES_SHARING_API_USER2); + // both group share and user share should be gone $this->assertFalse(\OC\Files\Filesystem::file_exists($this->filename)); - $this->assertTrue(\OC\Files\Filesystem::file_exists($duplicate)); // for user3 nothing should change self::loginHelper(self::TEST_FILES_SHARING_API_USER3); $this->assertTrue(\OC\Files\Filesystem::file_exists($this->filename)); - $this->assertFalse(\OC\Files\Filesystem::file_exists($duplicate)); + } + + /** + * if a file was shared as group share and as individual share they should be grouped + */ + function testGroupingOfShares() { + + $fileinfo = $this->view->getFileInfo($this->filename); + + $result = \OCP\Share::shareItem('file', $fileinfo['fileid'], \OCP\Share::SHARE_TYPE_GROUP, + \Test_Files_Sharing::TEST_FILES_SHARING_API_GROUP1, \OCP\PERMISSION_READ); + + $this->assertTrue($result); + + $result = \OCP\Share::shareItem('file', $fileinfo['fileid'], \OCP\Share::SHARE_TYPE_USER, + \Test_Files_Sharing::TEST_FILES_SHARING_API_USER2, \OCP\PERMISSION_UPDATE); + + $this->assertTrue($result); self::loginHelper(self::TEST_FILES_SHARING_API_USER2); - \OC\Files\Filesystem::unlink($duplicate); - self::loginHelper(self::TEST_FILES_SHARING_API_USER2); - $this->assertFalse(\OC\Files\Filesystem::file_exists($this->filename)); - $this->assertFalse(\OC\Files\Filesystem::file_exists($duplicate)); - // for user3 nothing should change - self::loginHelper(self::TEST_FILES_SHARING_API_USER3); - $this->assertTrue(\OC\Files\Filesystem::file_exists($this->filename)); - $this->assertFalse(\OC\Files\Filesystem::file_exists($duplicate)); + $result = \OCP\Share::getItemSharedWith('file', null); + + $this->assertTrue(is_array($result)); + + // test should return exactly one shares created from testCreateShare() + $this->assertSame(1, count($result)); + + $share = reset($result); + $this->assertSame(\OCP\PERMISSION_READ | \OCP\PERMISSION_UPDATE, $share['permissions']); + + \OC\Files\Filesystem::rename($this->filename, $this->filename . '-renamed'); + + $result = \OCP\Share::getItemSharedWith('file', null); + + $this->assertTrue(is_array($result)); + + // test should return exactly one shares created from testCreateShare() + $this->assertSame(1, count($result)); + + $share = reset($result); + $this->assertSame(\OCP\PERMISSION_READ | \OCP\PERMISSION_UPDATE, $share['permissions']); + $this->assertSame($this->filename . '-renamed', $share['file_target']); - //cleanup self::loginHelper(self::TEST_FILES_SHARING_API_USER1); - \OCP\Share::unshare('file', $fileinfo['fileid'], \OCP\Share::SHARE_TYPE_GROUP, - 'testGroup'); - \OCP\Share::unshare('file', $fileinfo['fileid'], \OCP\Share::SHARE_TYPE_USER, - self::TEST_FILES_SHARING_API_USER2); - \OC_Group::removeFromGroup(self::TEST_FILES_SHARING_API_USER2, 'testGroup'); - \OC_Group::removeFromGroup(self::TEST_FILES_SHARING_API_USER2, 'testGroup'); - \OC_Group::deleteGroup('testGroup'); + // unshare user share + $result = \OCP\Share::unshare('file', $fileinfo['fileid'], \OCP\Share::SHARE_TYPE_USER, + \Test_Files_Sharing::TEST_FILES_SHARING_API_USER2); + $this->assertTrue($result); - } + self::loginHelper(self::TEST_FILES_SHARING_API_USER2); + + $result = \OCP\Share::getItemSharedWith('file', null); + + $this->assertTrue(is_array($result)); + + // test should return the remaining group share + $this->assertSame(1, count($result)); + + $share = reset($result); + // only the group share permissions should be available now + $this->assertSame(\OCP\PERMISSION_READ, $share['permissions']); + $this->assertSame($this->filename . '-renamed', $share['file_target']); + + } function testShareWithDifferentShareFolder() { @@ -146,12 +187,6 @@ class Test_Files_Sharing extends Test_Files_Sharing_Base { $this->assertTrue(\OC\Files\Filesystem::file_exists('/Shared/subfolder/' . $this->folder)); //cleanup - self::loginHelper(self::TEST_FILES_SHARING_API_USER1); - \OCP\Share::unshare('file', $fileinfo['fileid'], \OCP\Share::SHARE_TYPE_USER, - self::TEST_FILES_SHARING_API_USER2); - \OCP\Share::unshare('folder', $folderinfo['fileid'], \OCP\Share::SHARE_TYPE_USER, - self::TEST_FILES_SHARING_API_USER2); - \OCP\Config::deleteSystemValue('share_folder'); } @@ -173,18 +208,14 @@ class Test_Files_Sharing extends Test_Files_Sharing_Base { $this->assertTrue(is_array($result)); // test should return exactly one shares created from testCreateShare() - $this->assertTrue(count($result) === 1); + $this->assertSame(1, count($result), 'more then one share found'); $share = reset($result); $this->assertSame($expectedPermissions, $share['permissions']); - - \OCP\Share::unshare('file', $fileinfo['fileid'], \OCP\Share::SHARE_TYPE_USER, - \Test_Files_Sharing::TEST_FILES_SHARING_API_USER2); } function DataProviderTestFileSharePermissions() { $permission1 = \OCP\PERMISSION_ALL; - $permission2 = \OCP\PERMISSION_DELETE; $permission3 = \OCP\PERMISSION_READ; $permission4 = \OCP\PERMISSION_READ | \OCP\PERMISSION_UPDATE; $permission5 = \OCP\PERMISSION_READ | \OCP\PERMISSION_DELETE; @@ -192,7 +223,6 @@ class Test_Files_Sharing extends Test_Files_Sharing_Base { return array( array($permission1, \OCP\PERMISSION_ALL & ~\OCP\PERMISSION_DELETE), - array($permission2, 0), array($permission3, $permission3), array($permission4, $permission4), array($permission5, $permission3), diff --git a/lib/private/share/share.php b/lib/private/share/share.php index 2864e8532aa..44051409504 100644 --- a/lib/private/share/share.php +++ b/lib/private/share/share.php @@ -1512,7 +1512,7 @@ class Share extends \OC\Share\Constants { * @param string $itemType * @return array of grouped items */ - private static function groupItems($items, $itemType) { + protected static function groupItems($items, $itemType) { $fileSharing = ($itemType === 'file' || $itemType === 'folder') ? true : false; diff --git a/tests/lib/share/share.php b/tests/lib/share/share.php index bb827eece73..a0b3643837e 100644 --- a/tests/lib/share/share.php +++ b/tests/lib/share/share.php @@ -522,7 +522,7 @@ class Test_Share extends PHPUnit_Framework_TestCase { OC_User::setUserId($this->user2); $this->assertEquals(array(OCP\PERMISSION_READ | OCP\PERMISSION_UPDATE), OCP\Share::getItemSharedWith('test', 'test.txt', Test_Share_Backend::FORMAT_PERMISSIONS)); OC_User::setUserId($this->user4); - $this->assertEquals(array(), OCP\Share::getItemsSharedWith('test', Test_Share_Backend::FORMAT_TARGET)); + $this->assertEquals(array('test.txt'), OCP\Share::getItemsSharedWith('test', Test_Share_Backend::FORMAT_TARGET)); // Valid share with same person - group then user OC_User::setUserId($this->user1); @@ -733,20 +733,88 @@ class Test_Share extends PHPUnit_Framework_TestCase { array(false, array('share_with' => '1234567890', 'share_type' => '3', 'id' => 101)), array(false, array('share_with' => '1234567890', 'share_type' => 3, 'id' => 101)), ); + } - /* - if (!isset($linkItem['share_with'])) { - return true; - } + /** + * @dataProvider dataProviderTestGroupItems + * @param type $ungrouped + * @param type $grouped + */ + function testGroupItems($ungrouped, $grouped) { - if ($linkItem['share_type'] != \OCP\Share::SHARE_TYPE_LINK) { - return true; - } + $result = DummyShareClass::groupItemsTest($ungrouped); - if ( \OC::$session->exists('public_link_authenticated') - && \OC::$session->get('public_link_authenticated') === $linkItem['id'] ) { - return true; + $this->compareArrays($grouped, $result); + + } + + function compareArrays($result, $expectedResult) { + foreach ($expectedResult as $key => $value) { + if (is_array($value)) { + $this->compareArrays($result[$key], $value); + } else { + $this->assertSame($value, $result[$key]); + } } - * */ + } + + function dataProviderTestGroupItems() { + return array( + // one array with one share + array( + array( // input + array('item_source' => 1, 'permissions' => \OCP\PERMISSION_ALL, 'item_target' => 't1')), + array( // expected result + array('item_source' => 1, 'permissions' => \OCP\PERMISSION_ALL, 'item_target' => 't1'))), + // two shares both point to the same source + array( + array( // input + array('item_source' => 1, 'permissions' => \OCP\PERMISSION_READ, 'item_target' => 't1'), + array('item_source' => 1, 'permissions' => \OCP\PERMISSION_UPDATE, 'item_target' => 't1'), + ), + array( // expected result + array('item_source' => 1, 'permissions' => \OCP\PERMISSION_READ | \OCP\PERMISSION_UPDATE, 'item_target' => 't1', + 'grouped' => array( + array('item_source' => 1, 'permissions' => \OCP\PERMISSION_READ, 'item_target' => 't1'), + array('item_source' => 1, 'permissions' => \OCP\PERMISSION_UPDATE, 'item_target' => 't1'), + ) + ), + ) + ), + // two shares both point to the same source but with different targets + array( + array( // input + array('item_source' => 1, 'permissions' => \OCP\PERMISSION_READ, 'item_target' => 't1'), + array('item_source' => 1, 'permissions' => \OCP\PERMISSION_UPDATE, 'item_target' => 't2'), + ), + array( // expected result + array('item_source' => 1, 'permissions' => \OCP\PERMISSION_READ, 'item_target' => 't1'), + array('item_source' => 1, 'permissions' => \OCP\PERMISSION_UPDATE, 'item_target' => 't2'), + ) + ), + // three shares two point to the same source + array( + array( // input + array('item_source' => 1, 'permissions' => \OCP\PERMISSION_READ, 'item_target' => 't1'), + array('item_source' => 2, 'permissions' => \OCP\PERMISSION_CREATE, 'item_target' => 't2'), + array('item_source' => 1, 'permissions' => \OCP\PERMISSION_UPDATE, 'item_target' => 't1'), + ), + array( // expected result + array('item_source' => 1, 'permissions' => \OCP\PERMISSION_READ | \OCP\PERMISSION_UPDATE, 'item_target' => 't1', + 'grouped' => array( + array('item_source' => 1, 'permissions' => \OCP\PERMISSION_READ, 'item_target' => 't1'), + array('item_source' => 1, 'permissions' => \OCP\PERMISSION_UPDATE, 'item_target' => 't1'), + ) + ), + array('item_source' => 2, 'permissions' => \OCP\PERMISSION_CREATE, 'item_target' => 't2'), + ) + ), + ); + } +} + +class DummyShareClass extends \OC\Share\Share { + public static function groupItemsTest($items) { + return parent::groupItems($items, 'test'); } } -- cgit v1.2.3 From 97dba2e022aa1d2431b97efa01ec158bf3979040 Mon Sep 17 00:00:00 2001 From: Bjoern Schiessle Date: Fri, 22 Aug 2014 15:59:44 +0200 Subject: generateTarget() will always find a unique target --- lib/private/share/helper.php | 117 +++++++++++++++---------------------------- lib/private/share/share.php | 59 +++++++++++++++------- 2 files changed, 80 insertions(+), 96 deletions(-) diff --git a/lib/private/share/helper.php b/lib/private/share/helper.php index 46e3c280488..0c679b2bda4 100644 --- a/lib/private/share/helper.php +++ b/lib/private/share/helper.php @@ -35,8 +35,7 @@ class Helper extends \OC\Share\Constants { * @throws \Exception * @return string Item target */ - public static function generateTarget($itemType, $itemSource, $shareType, $shareWith, $uidOwner, - $suggestedTarget = null, $groupParent = null) { + public static function generateTarget($itemType, $itemSource, $shareType, $shareWith, $uidOwner, $suggestedTarget = null, $groupParent = null) { $backend = \OC\Share\Share::getBackend($itemType); if ($shareType == self::SHARE_TYPE_LINK) { if (isset($suggestedTarget)) { @@ -58,87 +57,51 @@ class Helper extends \OC\Share\Constants { } else { $userAndGroups = false; } - $exclude = null; - // Backend has 3 opportunities to generate a unique target - for ($i = 0; $i < 2; $i++) { - // Check if suggested target exists first - if ($i == 0 && isset($suggestedTarget)) { - $target = $suggestedTarget; + $exclude = array(); + // Find similar targets to improve backend's chances to generate a unqiue target + if ($userAndGroups) { + if ($column == 'file_target') { + $checkTargets = \OC_DB::prepare('SELECT `' . $column . '` FROM `*PREFIX*share`' + . ' WHERE `item_type` IN (\'file\', \'folder\')' + . ' AND `share_type` IN (?,?,?)' + . ' AND `share_with` IN (\'' . implode('\',\'', $userAndGroups) . '\')'); + $result = $checkTargets->execute(array(self::SHARE_TYPE_USER, self::SHARE_TYPE_GROUP, + self::$shareTypeGroupUserUnique)); } else { - if ($shareType == self::SHARE_TYPE_GROUP) { - $target = $backend->generateTarget($itemSource, false, $exclude); - } else { - $target = $backend->generateTarget($itemSource, $shareWith, $exclude); - } - if (is_array($exclude) && in_array($target, $exclude)) { - break; - } + $checkTargets = \OC_DB::prepare('SELECT `' . $column . '` FROM `*PREFIX*share`' + . ' WHERE `item_type` = ? AND `share_type` IN (?,?,?)' + . ' AND `share_with` IN (\'' . implode('\',\'', $userAndGroups) . '\')'); + $result = $checkTargets->execute(array($itemType, self::SHARE_TYPE_USER, + self::SHARE_TYPE_GROUP, self::$shareTypeGroupUserUnique)); } - // Check if target already exists - $checkTarget = \OC\Share\Share::getItems($itemType, $target, $shareType, $shareWith); - if (!empty($checkTarget)) { - foreach ($checkTarget as $item) { - // Skip item if it is the group parent row - if (isset($groupParent) && $item['id'] == $groupParent) { - if (count($checkTarget) == 1) { - return $target; - } else { - continue; - } - } - if ($item['uid_owner'] == $uidOwner) { - if ($itemType == 'file' || $itemType == 'folder') { - $meta = \OC\Files\Filesystem::getFileInfo($itemSource); - if ($item['file_source'] == $meta['fileid']) { - return $target; - } - } else if ($item['item_source'] == $itemSource) { - return $target; - } - } - } - if (!isset($exclude)) { - $exclude = array(); - } - // Find similar targets to improve backend's chances to generate a unqiue target - if ($userAndGroups) { - if ($column == 'file_target') { - $checkTargets = \OC_DB::prepare('SELECT `'.$column.'` FROM `*PREFIX*share`' - .' WHERE `item_type` IN (\'file\', \'folder\')' - .' AND `share_type` IN (?,?,?)' - .' AND `share_with` IN (\''.implode('\',\'', $userAndGroups).'\')'); - $result = $checkTargets->execute(array(self::SHARE_TYPE_USER, self::SHARE_TYPE_GROUP, - self::$shareTypeGroupUserUnique)); - } else { - $checkTargets = \OC_DB::prepare('SELECT `'.$column.'` FROM `*PREFIX*share`' - .' WHERE `item_type` = ? AND `share_type` IN (?,?,?)' - .' AND `share_with` IN (\''.implode('\',\'', $userAndGroups).'\')'); - $result = $checkTargets->execute(array($itemType, self::SHARE_TYPE_USER, - self::SHARE_TYPE_GROUP, self::$shareTypeGroupUserUnique)); - } - } else { - if ($column == 'file_target') { - $checkTargets = \OC_DB::prepare('SELECT `'.$column.'` FROM `*PREFIX*share`' - .' WHERE `item_type` IN (\'file\', \'folder\')' - .' AND `share_type` = ? AND `share_with` = ?'); - $result = $checkTargets->execute(array(self::SHARE_TYPE_GROUP, $shareWith)); - } else { - $checkTargets = \OC_DB::prepare('SELECT `'.$column.'` FROM `*PREFIX*share`' - .' WHERE `item_type` = ? AND `share_type` = ? AND `share_with` = ?'); - $result = $checkTargets->execute(array($itemType, self::SHARE_TYPE_GROUP, $shareWith)); - } - } - while ($row = $result->fetchRow()) { - $exclude[] = $row[$column]; - } + } else { + if ($column == 'file_target') { + $checkTargets = \OC_DB::prepare('SELECT `' . $column . '` FROM `*PREFIX*share`' + . ' WHERE `item_type` IN (\'file\', \'folder\')' + . ' AND `share_type` = ? AND `share_with` = ?'); + $result = $checkTargets->execute(array(self::SHARE_TYPE_GROUP, $shareWith)); } else { - return $target; + $checkTargets = \OC_DB::prepare('SELECT `' . $column . '` FROM `*PREFIX*share`' + . ' WHERE `item_type` = ? AND `share_type` = ? AND `share_with` = ?'); + $result = $checkTargets->execute(array($itemType, self::SHARE_TYPE_GROUP, $shareWith)); } } + while ($row = $result->fetchRow()) { + $exclude[] = $row[$column]; + } + + // Check if suggested target exists first + if (!isset($suggestedTarget)) { + $suggestedTarget = $itemSource; + } + if ($shareType == self::SHARE_TYPE_GROUP) { + $target = $backend->generateTarget($suggestedTarget, false, $exclude); + } else { + $target = $backend->generateTarget($suggestedTarget, $shareWith, $exclude); + } + + return $target; } - $message = 'Sharing backend registered for '.$itemType.' did not generate a unique target for '.$itemSource; - \OC_Log::write('OCP\Share', $message, \OC_Log::ERROR); - throw new \Exception($message); } /** diff --git a/lib/private/share/share.php b/lib/private/share/share.php index 44051409504..c16ef5af74d 100644 --- a/lib/private/share/share.php +++ b/lib/private/share/share.php @@ -296,7 +296,7 @@ class Share extends \OC\Share\Constants { // first check if there is a db entry for the specific user $query = \OC_DB::prepare( - 'SELECT `file_target`, `permissions`, `expiration` + 'SELECT `file_target`, `item_target`, `permissions`, `expiration` FROM `*PREFIX*share` WHERE @@ -1290,13 +1290,17 @@ class Share extends \OC\Share\Constants { $queryArgs = array_merge($queryArgs, $collectionTypes); } } + + if ($shareType == self::$shareTypeUserAndGroups) { + // Make sure the unique user target is returned if it exists, + // unique targets should follow the group share in the database + // If the limit is not 1, the filtering can be done later + $where .= ' ORDER BY `*PREFIX*share`.`id` DESC'; + } else { + $where .= ' ORDER BY `*PREFIX*share`.`id` ASC'; + } + if ($limit != -1 && !$includeCollections) { - if ($shareType == self::$shareTypeUserAndGroups) { - // Make sure the unique user target is returned if it exists, - // unique targets should follow the group share in the database - // If the limit is not 1, the filtering can be done later - $where .= ' ORDER BY `*PREFIX*share`.`id` ASC'; - } // The limit must be at least 3, because filtering needs to be done if ($limit < 3) { $queryLimit = 3; @@ -1305,7 +1309,6 @@ class Share extends \OC\Share\Constants { } } else { $queryLimit = null; - $where .= ' ORDER BY `*PREFIX*share`.`id` ASC'; } $select = self::createSelectStatement($format, $fileDependent, $uidOwner); $root = strlen($root); @@ -1334,10 +1337,10 @@ class Share extends \OC\Share\Constants { } } else if (!isset($uidOwner)) { // Check if the same target already exists - if (isset($targets[$row[$column]])) { + if (isset($targets[$row['id']])) { // Check if the same owner shared with the user twice // through a group and user share - this is allowed - $id = $targets[$row[$column]]; + $id = $targets[$row['id']]; if (isset($items[$id]) && $items[$id]['uid_owner'] == $row['uid_owner']) { // Switch to group share type to ensure resharing conditions aren't bypassed if ($items[$id]['share_type'] != self::SHARE_TYPE_GROUP) { @@ -1353,12 +1356,12 @@ class Share extends \OC\Share\Constants { unset($items[$id]); $id = $row['id']; } - // Combine the permissions for the item $items[$id]['permissions'] |= (int)$row['permissions']; - continue; + } - } else { - $targets[$row[$column]] = $row['id']; + continue; + } elseif (!empty($row['parent'])) { + $targets[$row['parent']] = $row['id']; } } // Remove root from file source paths if retrieving own shared items @@ -1413,10 +1416,13 @@ class Share extends \OC\Share\Constants { $row['displayname_owner'] = \OCP\User::getDisplayName($row['uid_owner']); } - $items[$row['id']] = $row; + if ($row['permissions'] > 0) { + $items[$row['id']] = $row; + } } + // group items if we are looking for items shared with the current user if (isset($shareWith) && $shareWith === \OCP\User::getUser()) { $items = self::groupItems($items, $itemType); } @@ -1642,7 +1648,8 @@ class Share extends \OC\Share\Constants { } foreach ($users as $user) { - $sourceExists = self::getItemSharedWithBySource($itemType, $itemSource, self::FORMAT_NONE, null, true, $user); + $sourceId = ($itemType === 'file' || $itemType === 'folder') ? $fileSource : $itemSource; + $sourceExists = self::getItemSharedWithBySource($itemType, $sourceId, self::FORMAT_NONE, null, true, $user); $shareType = ($isGroupShare) ? self::$shareTypeGroupUserUnique : $shareType; @@ -1675,9 +1682,23 @@ class Share extends \OC\Share\Constants { } } else { - // move to the next user if it is neither a unique group share - // nor a single share without adding it to the share table - continue; + + // group share which doesn't exists until now, check if we need a unique target for this user + + $itemTarget = Helper::generateTarget($itemType, $itemSource, self::SHARE_TYPE_USER, $user, + $uidOwner, $suggestedItemTarget, $parent); + + // do we also need a file target + if (isset($fileSource)) { + $fileTarget = Helper::generateTarget('file', $filePath, self::SHARE_TYPE_USER, $user, + $uidOwner, $suggestedFileTarget, $parent); + } else { + $fileTarget = null; + } + + if ($itemTarget === $groupItemTarget) { + continue; + } } $queriesToExecute[] = array( -- cgit v1.2.3 From df97d8299ad4d96d53807b871c6197e53d652e67 Mon Sep 17 00:00:00 2001 From: Bjoern Schiessle Date: Wed, 27 Aug 2014 00:30:13 +0200 Subject: fix add user to group to work with grouped shares --- lib/private/share/hooks.php | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/lib/private/share/hooks.php b/lib/private/share/hooks.php index 9ac64d888ea..2be7dc85e3c 100644 --- a/lib/private/share/hooks.php +++ b/lib/private/share/hooks.php @@ -45,6 +45,7 @@ class Hooks extends \OC\Share\Constants { * @param array $arguments */ public static function post_addToGroup($arguments) { + // Find the group shares and check if the user needs a unique target $query = \OC_DB::prepare('SELECT * FROM `*PREFIX*share` WHERE `share_type` = ? AND `share_with` = ?'); $result = $query->execute(array(self::SHARE_TYPE_GROUP, $arguments['gid'])); @@ -52,18 +53,25 @@ class Hooks extends \OC\Share\Constants { .' `item_target`, `parent`, `share_type`, `share_with`, `uid_owner`, `permissions`,' .' `stime`, `file_source`, `file_target`) VALUES (?,?,?,?,?,?,?,?,?,?,?)'); while ($item = $result->fetchRow()) { - if ($item['item_type'] == 'file' || $item['item_type'] == 'file') { - $itemTarget = null; - } else { - $itemTarget = Helper::generateTarget($item['item_type'], $item['item_source'], self::SHARE_TYPE_USER, - $arguments['uid'], $item['uid_owner'], $item['item_target'], $item['id']); - } - if (isset($item['file_source'])) { - $fileTarget = Helper::generateTarget($item['item_type'], $item['item_source'], self::SHARE_TYPE_USER, - $arguments['uid'], $item['uid_owner'], $item['file_target'], $item['id']); + + $sourceExists = \OC\Share\Share::getItemSharedWithBySource($item['item_type'], $item['item_source'], self::FORMAT_NONE, null, true, $arguments['uid']); + + if ($sourceExists) { + $fileTarget = $sourceExists['file_target']; + $itemTarget = $sourceExists['item_target']; } else { - $fileTarget = null; + $itemTarget = Helper::generateTarget($item['item_type'], $item['item_source'], self::SHARE_TYPE_USER, $arguments['uid'], + $item['owner'], null, $item['parent']); + + // do we also need a file target + if ($item['item_type'] === 'file' || $item['item_type'] === 'folder') { + $fileTarget = Helper::generateTarget('file', $row['file_target'], self::SHARE_TYPE_USER, $arguments['uid'], + $item['owner'], null, $item['parent']); + } else { + $fileTarget = null; + } } + // Insert an extra row for the group share if the item or file target is unique for this user if ($itemTarget != $item['item_target'] || $fileTarget != $item['file_target']) { $query->execute(array($item['item_type'], $item['item_source'], $itemTarget, $item['id'], -- cgit v1.2.3 From 98268078c0143d34317366b13aee2908371806ec Mon Sep 17 00:00:00 2001 From: Bjoern Schiessle Date: Wed, 27 Aug 2014 00:31:39 +0200 Subject: mark exclude list as deprecated. It neither used by the files app nor by the calendar or contacts app. It doesn't make sense to build a exclude list by the share API, the apps knows best which are valid targets. --- lib/public/share.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/public/share.php b/lib/public/share.php index bb9c6ec5886..bce7ace6741 100644 --- a/lib/public/share.php +++ b/lib/public/share.php @@ -361,7 +361,7 @@ interface Share_Backend { * Get a unique name of the item for the specified user * @param string $itemSource * @param string|false $shareWith User the item is being shared with - * @param array|null $exclude List of similar item names already existing as shared items + * @param array|null $exclude List of similar item names already existing as shared items @deprecated since version OC7 * @return string Target name * * This function needs to verify that the user does not already have an item with this name. -- cgit v1.2.3 From aab44694ad284e81b949a896e129d0488569d779 Mon Sep 17 00:00:00 2001 From: Bjoern Schiessle Date: Wed, 27 Aug 2014 00:31:49 +0200 Subject: fix unit tests --- tests/lib/share/backend.php | 23 ++++++++++++++++++++--- tests/lib/share/share.php | 4 ++-- 2 files changed, 22 insertions(+), 5 deletions(-) diff --git a/tests/lib/share/backend.php b/tests/lib/share/backend.php index 420bd9d88b3..50ce24e07b6 100644 --- a/tests/lib/share/backend.php +++ b/tests/lib/share/backend.php @@ -38,19 +38,36 @@ class Test_Share_Backend implements OCP\Share_Backend { public function generateTarget($itemSource, $shareWith, $exclude = null) { // Always make target be test.txt to cause conflicts - $target = 'test.txt'; - if (isset($exclude)) { + + if (substr($itemSource, 0, strlen('test')) !== 'test') { + $target = "test.txt"; + } else { + $target = $itemSource; + } + + + $shares = \OCP\Share::getItemsSharedWithUser('test', $shareWith); + + $knownTargets = array(); + foreach ($shares as $share) { + $knownTargets[] = $share['item_target']; + } + + + if (in_array($target, $knownTargets)) { $pos = strrpos($target, '.'); $name = substr($target, 0, $pos); $ext = substr($target, $pos); $append = ''; $i = 1; - while (in_array($name.$append.$ext, $exclude)) { + while (in_array($name.$append.$ext, $knownTargets)) { $append = $i; $i++; } $target = $name.$append.$ext; + } + return $target; } diff --git a/tests/lib/share/share.php b/tests/lib/share/share.php index a0b3643837e..fbff89567d2 100644 --- a/tests/lib/share/share.php +++ b/tests/lib/share/share.php @@ -48,8 +48,8 @@ class Test_Share extends PHPUnit_Framework_TestCase { OC_User::setUserId($this->user1); OC_Group::clearBackends(); OC_Group::useBackend(new OC_Group_Dummy); - $this->group1 = uniqid('group_'); - $this->group2 = uniqid('group_'); + $this->group1 = uniqid('group1_'); + $this->group2 = uniqid('group2_'); OC_Group::createGroup($this->group1); OC_Group::createGroup($this->group2); OC_Group::addToGroup($this->user1, $this->group1); -- cgit v1.2.3 From 7edf912ef8a342c62d2e82e9cdc713ec14ba6f18 Mon Sep 17 00:00:00 2001 From: Bjoern Schiessle Date: Thu, 18 Sep 2014 14:46:11 +0200 Subject: only add a new row if it isn't a unique share, otherwise update the existing row --- lib/private/share/share.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/private/share/share.php b/lib/private/share/share.php index c16ef5af74d..6cfa7a78ff0 100644 --- a/lib/private/share/share.php +++ b/lib/private/share/share.php @@ -771,7 +771,7 @@ class Share extends \OC\Share\Constants { } } - if (!$itemUnshared && isset($groupShare)) { + if (!$itemUnshared && isset($groupShare) && !isset($uniqueGroupShare)) { $query = \OC_DB::prepare('INSERT INTO `*PREFIX*share`' .' (`item_type`, `item_source`, `item_target`, `parent`, `share_type`,' .' `share_with`, `uid_owner`, `permissions`, `stime`, `file_source`, `file_target`)' -- cgit v1.2.3 From e25593db4ef9d054d1fbbc306589945c09e6bef7 Mon Sep 17 00:00:00 2001 From: Bjoern Schiessle Date: Mon, 22 Sep 2014 18:13:40 +0200 Subject: clear share table after each test run --- apps/files_sharing/tests/base.php | 3 +++ 1 file changed, 3 insertions(+) diff --git a/apps/files_sharing/tests/base.php b/apps/files_sharing/tests/base.php index 60159d84e7e..738ba3493ba 100644 --- a/apps/files_sharing/tests/base.php +++ b/apps/files_sharing/tests/base.php @@ -92,6 +92,9 @@ abstract class Test_Files_Sharing_Base extends \PHPUnit_Framework_TestCase { } else { \OC_App::disable('files_encryption'); } + + $query = \OCP\DB::prepare('DELETE FROM `*PREFIX*share`'); + $query->execute(); } public static function tearDownAfterClass() { -- cgit v1.2.3 From 27a630b0bcf2a75fd3520638ff5f94366a6cf44d Mon Sep 17 00:00:00 2001 From: Bjoern Schiessle Date: Thu, 25 Sep 2014 11:29:57 +0200 Subject: some small fixes --- apps/files_sharing/lib/sharedmount.php | 6 ++---- apps/files_sharing/lib/sharedstorage.php | 2 ++ lib/private/share/share.php | 7 +------ lib/public/share.php | 2 +- 4 files changed, 6 insertions(+), 11 deletions(-) diff --git a/apps/files_sharing/lib/sharedmount.php b/apps/files_sharing/lib/sharedmount.php index 9469e0aa48d..1717a4bd636 100644 --- a/apps/files_sharing/lib/sharedmount.php +++ b/apps/files_sharing/lib/sharedmount.php @@ -58,7 +58,7 @@ class SharedMount extends Mount implements MoveableMount { * update fileTarget in the database if the mount point changed * @param string $newPath * @param array $share reference to the share which should be modified - * @return type + * @return bool */ private static function updateFileTarget($newPath, &$share) { // if the user renames a mount point from a group share we need to create a new db entry @@ -152,9 +152,7 @@ class SharedMount extends Mount implements MoveableMount { */ public function removeMount() { $mountManager = \OC\Files\Filesystem::getMountManager(); - /** - * @var \OC\Files\Storage\Shared - */ + /** @var \OC\Files\Storage\Shared */ $storage = $this->getStorage(); $result = $storage->unshareStorage(); $mountManager->removeMount($this->mountPoint); diff --git a/apps/files_sharing/lib/sharedstorage.php b/apps/files_sharing/lib/sharedstorage.php index 0bd7c6f4fd5..dc161616110 100644 --- a/apps/files_sharing/lib/sharedstorage.php +++ b/apps/files_sharing/lib/sharedstorage.php @@ -534,6 +534,8 @@ class Shared extends \OC\Files\Storage\Common implements ISharedStorage { /** * unshare complete storage, also the grouped shares + * + * @return bool */ public function unshareStorage() { $result = true; diff --git a/lib/private/share/share.php b/lib/private/share/share.php index 6cfa7a78ff0..0234acf23a8 100644 --- a/lib/private/share/share.php +++ b/lib/private/share/share.php @@ -340,7 +340,7 @@ class Share extends \OC\Share\Constants { * @param mixed $parameters * @param boolean $includeCollections * @param string $shareWith (optional) define against which user should be checked, default: current user - * @return mixed Return depends on format + * @return array */ public static function getItemSharedWithBySource($itemType, $itemSource, $format = self::FORMAT_NONE, $parameters = null, $includeCollections = false, $shareWith = null) { @@ -1572,11 +1572,6 @@ class Share extends \OC\Share\Constants { $queriesToExecute = array(); - $fileShare = false; - if ($itemType === 'file' || $itemType === 'folder') { - $fileShare = true; - } - $result = self::checkReshare($itemType, $itemSource, $shareType, $shareWith, $uidOwner, $permissions, $itemSourceName, $expirationDate); if(!empty($result)) { $parent = $result['parent']; diff --git a/lib/public/share.php b/lib/public/share.php index bce7ace6741..8ce389a1ed7 100644 --- a/lib/public/share.php +++ b/lib/public/share.php @@ -139,7 +139,7 @@ class Share extends \OC\Share\Constants { * @param int $format (optional) Format type must be defined by the backend * @param mixed $parameters * @param bool $includeCollections - * @return mixed Return depends on format + * @return array */ public static function getItemSharedWithBySource($itemType, $itemSource, $format = self::FORMAT_NONE, $parameters = null, $includeCollections = false) { -- cgit v1.2.3 From c5db1ef3ccee153361966fe7b43e967bf62e004b Mon Sep 17 00:00:00 2001 From: Bjoern Schiessle Date: Thu, 25 Sep 2014 12:35:11 +0200 Subject: always select permissions, used in getItems() --- lib/private/share/share.php | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/private/share/share.php b/lib/private/share/share.php index 0234acf23a8..aa64e3ff45d 100644 --- a/lib/private/share/share.php +++ b/lib/private/share/share.php @@ -1399,6 +1399,7 @@ class Share extends \OC\Share\Constants { } } } + if($checkExpireDate) { if (self::expireItem($row)) { continue; @@ -1899,9 +1900,9 @@ class Share extends \OC\Share\Constants { $select = '*'; if ($format == self::FORMAT_STATUSES) { if ($fileDependent) { - $select = '`*PREFIX*share`.`id`, `*PREFIX*share`.`parent`, `share_type`, `path`, `storage`, `share_with`, `uid_owner` , `file_source`, `stime`'; + $select = '`*PREFIX*share`.`id`, `*PREFIX*share`.`parent`, `share_type`, `path`, `storage`, `share_with`, `uid_owner` , `file_source`, `stime`, `*PREFIX*share`.`permissions`'; } else { - $select = '`id`, `parent`, `share_type`, `share_with`, `uid_owner`, `item_source`, `stime`'; + $select = '`id`, `parent`, `share_type`, `share_with`, `uid_owner`, `item_source`, `stime`, `*PREFIX*share`.`permissions`'; } } else { if (isset($uidOwner)) { -- cgit v1.2.3 From 6d747e97213a3440e11d41bf43743b97cbf81c04 Mon Sep 17 00:00:00 2001 From: Bjoern Schiessle Date: Thu, 25 Sep 2014 13:45:23 +0200 Subject: call \OCP\Share::getItemsSharedWithUser() to get exclude list, this way all checks are executed, e.g. to check if the share is really visible --- apps/files_sharing/lib/share/file.php | 7 +--- apps/files_sharing/tests/share.php | 60 +++++++++++++++++++++++++++++++++++ lib/private/share/helper.php | 34 +++----------------- 3 files changed, 66 insertions(+), 35 deletions(-) diff --git a/apps/files_sharing/lib/share/file.php b/apps/files_sharing/lib/share/file.php index 0cd66547d0b..1086e21d79f 100644 --- a/apps/files_sharing/lib/share/file.php +++ b/apps/files_sharing/lib/share/file.php @@ -83,12 +83,7 @@ class OC_Share_Backend_File implements OCP\Share_Backend_File_Dependent { } } - $excludeList = \OCP\Share::getItemsSharedWithUser('file', $shareWith, self::FORMAT_TARGET_NAMES); - if (is_array($exclude)) { - $excludeList = array_merge($excludeList, $exclude); - } - - return \OCA\Files_Sharing\Helper::generateUniqueTarget($target, $excludeList, $view); + return \OCA\Files_Sharing\Helper::generateUniqueTarget($target, $exclude, $view); } public function formatItems($items, $format, $parameters = null) { diff --git a/apps/files_sharing/tests/share.php b/apps/files_sharing/tests/share.php index c780a9e816a..80199505d89 100644 --- a/apps/files_sharing/tests/share.php +++ b/apps/files_sharing/tests/share.php @@ -164,7 +164,67 @@ class Test_Files_Sharing extends Test_Files_Sharing_Base { $this->assertSame(\OCP\PERMISSION_READ, $share['permissions']); $this->assertSame($this->filename . '-renamed', $share['file_target']); + } + + /** + * user1 share file to a group and to a user2 in the same group. Then user2 + * unshares the file from self. Afterwards user1 should no longer see the + * 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() { + $fileinfo = $this->view->getFileInfo($this->filename); + + // share the file to group1 (user2 is a member of this group) and explicitely to user2 + \OCP\Share::shareItem('file', $fileinfo['fileid'], \OCP\Share::SHARE_TYPE_GROUP, self::TEST_FILES_SHARING_API_GROUP1, \OCP\PERMISSION_ALL); + \OCP\Share::shareItem('file', $fileinfo['fileid'], \OCP\Share::SHARE_TYPE_USER, self::TEST_FILES_SHARING_API_USER2, \OCP\PERMISSION_ALL); + + // user1 should have to shared files + $shares = \OCP\Share::getItemsShared('file'); + $this->assertSame(2, count($shares)); + + // user2 should have two files "welcome.txt" and the shared file, + // both the group share and the single share of the same file should be + // grouped to one file + \Test_Files_Sharing::loginHelper(self::TEST_FILES_SHARING_API_USER2); + $dirContent = \OC\Files\Filesystem::getDirectoryContent('/'); + $this->assertSame(2, count($dirContent)); + $this->verifyDirContent($dirContent, array('welcome.txt', ltrim($this->filename, '/'))); + + // now user2 deletes the share (= unshare from self) + \OC\Files\Filesystem::unlink($this->filename); + + // only welcome.txt should exists + $dirContent = \OC\Files\Filesystem::getDirectoryContent('/'); + $this->assertSame(1, count($dirContent)); + $this->verifyDirContent($dirContent, array('welcome.txt')); + + // login as user1... + \Test_Files_Sharing::loginHelper(self::TEST_FILES_SHARING_API_USER1); + + // ... now user1 should have only one shared file, the group share + $shares = \OCP\Share::getItemsShared('file'); + $this->assertSame(1, count($shares)); + + // user1 shares a gain the file directly to user2 + \OCP\Share::shareItem('file', $fileinfo['fileid'], \OCP\Share::SHARE_TYPE_USER, self::TEST_FILES_SHARING_API_USER2, \OCP\PERMISSION_ALL); + + // user2 should see again welcome.txt and the shared file + \Test_Files_Sharing::loginHelper(self::TEST_FILES_SHARING_API_USER2); + $dirContent = \OC\Files\Filesystem::getDirectoryContent('/'); + $this->assertSame(2, count($dirContent)); + $this->verifyDirContent($dirContent, array('welcome.txt', ltrim($this->filename, '/'))); + + + } + + 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']); + } } + } function testShareWithDifferentShareFolder() { diff --git a/lib/private/share/helper.php b/lib/private/share/helper.php index 0c679b2bda4..7e1cbb273b8 100644 --- a/lib/private/share/helper.php +++ b/lib/private/share/helper.php @@ -58,37 +58,13 @@ class Helper extends \OC\Share\Constants { $userAndGroups = false; } $exclude = array(); - // Find similar targets to improve backend's chances to generate a unqiue target - if ($userAndGroups) { - if ($column == 'file_target') { - $checkTargets = \OC_DB::prepare('SELECT `' . $column . '` FROM `*PREFIX*share`' - . ' WHERE `item_type` IN (\'file\', \'folder\')' - . ' AND `share_type` IN (?,?,?)' - . ' AND `share_with` IN (\'' . implode('\',\'', $userAndGroups) . '\')'); - $result = $checkTargets->execute(array(self::SHARE_TYPE_USER, self::SHARE_TYPE_GROUP, - self::$shareTypeGroupUserUnique)); - } else { - $checkTargets = \OC_DB::prepare('SELECT `' . $column . '` FROM `*PREFIX*share`' - . ' WHERE `item_type` = ? AND `share_type` IN (?,?,?)' - . ' AND `share_with` IN (\'' . implode('\',\'', $userAndGroups) . '\')'); - $result = $checkTargets->execute(array($itemType, self::SHARE_TYPE_USER, - self::SHARE_TYPE_GROUP, self::$shareTypeGroupUserUnique)); - } - } else { - if ($column == 'file_target') { - $checkTargets = \OC_DB::prepare('SELECT `' . $column . '` FROM `*PREFIX*share`' - . ' WHERE `item_type` IN (\'file\', \'folder\')' - . ' AND `share_type` = ? AND `share_with` = ?'); - $result = $checkTargets->execute(array(self::SHARE_TYPE_GROUP, $shareWith)); - } else { - $checkTargets = \OC_DB::prepare('SELECT `' . $column . '` FROM `*PREFIX*share`' - . ' WHERE `item_type` = ? AND `share_type` = ? AND `share_with` = ?'); - $result = $checkTargets->execute(array($itemType, self::SHARE_TYPE_GROUP, $shareWith)); + + $result = \OCP\Share::getItemsSharedWithUser($itemType, $shareWith); + foreach ($result as $row) { + if ($row['permissions'] > 0) { + $exclude[] = $row[$column]; } } - while ($row = $result->fetchRow()) { - $exclude[] = $row[$column]; - } // Check if suggested target exists first if (!isset($suggestedTarget)) { -- cgit v1.2.3 From 3b1715c3fc718a66a6ab41b762b6550748669e27 Mon Sep 17 00:00:00 2001 From: Bjoern Schiessle Date: Fri, 26 Sep 2014 13:01:54 +0200 Subject: for group shares only the parent has the up-to-date permission. Make sure that we always use this permission, except if the user permission is '0' because in this case the user unshared the group share from self --- lib/private/share/share.php | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/lib/private/share/share.php b/lib/private/share/share.php index aa64e3ff45d..3bae382d34a 100644 --- a/lib/private/share/share.php +++ b/lib/private/share/share.php @@ -1291,7 +1291,7 @@ class Share extends \OC\Share\Constants { } } - if ($shareType == self::$shareTypeUserAndGroups) { + if ($shareType == self::$shareTypeUserAndGroups && $limit === 1) { // Make sure the unique user target is returned if it exists, // unique targets should follow the group share in the database // If the limit is not 1, the filtering can be done later @@ -1330,6 +1330,12 @@ class Share extends \OC\Share\Constants { $row['share_type'] = self::SHARE_TYPE_GROUP; $row['unique_name'] = true; // remember that we use a unique name for this user $row['share_with'] = $items[$row['parent']]['share_with']; + // if the group share was unshared from the user we keep the permission, otherwise + // we take the permission from the parent because this is always the up-to-date + // permission for the group share + if ($row['permissions'] > 0) { + $row['permissions'] = $items[$row['parent']]['permissions']; + } // Remove the parent group share unset($items[$row['parent']]); if ($row['permissions'] == 0) { @@ -1653,6 +1659,11 @@ class Share extends \OC\Share\Constants { $fileTarget = $sourceExists['file_target']; $itemTarget = $sourceExists['item_target']; + // for group shares we don't need a additional entry if the target is the same + //if($isGroupShare && $groupItemTarget === $itemTarget) { + // continue; + //} + } elseif(!$sourceExists && !$isGroupShare) { $itemTarget = Helper::generateTarget($itemType, $itemSource, self::SHARE_TYPE_USER, $user, -- cgit v1.2.3 From 6d85cf2a0b021a9406a979c2d791ab665f275be9 Mon Sep 17 00:00:00 2001 From: Bjoern Schiessle Date: Fri, 26 Sep 2014 13:10:31 +0200 Subject: for group shares we don't need a extra db entry of groupTarget equals itemTarget --- lib/private/share/share.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/private/share/share.php b/lib/private/share/share.php index 3bae382d34a..efd8a220bb8 100644 --- a/lib/private/share/share.php +++ b/lib/private/share/share.php @@ -1660,9 +1660,9 @@ class Share extends \OC\Share\Constants { $itemTarget = $sourceExists['item_target']; // for group shares we don't need a additional entry if the target is the same - //if($isGroupShare && $groupItemTarget === $itemTarget) { - // continue; - //} + if($isGroupShare && $groupItemTarget === $itemTarget) { + continue; + } } elseif(!$sourceExists && !$isGroupShare) { -- cgit v1.2.3 From 1e5c786392c9f5098a328ee0294ec3dd526308ed Mon Sep 17 00:00:00 2001 From: Bjoern Schiessle Date: Fri, 26 Sep 2014 13:19:15 +0200 Subject: only create a new share entry, if the user needs a different target name than the group share --- lib/private/share/share.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/private/share/share.php b/lib/private/share/share.php index efd8a220bb8..bb0217c0a4c 100644 --- a/lib/private/share/share.php +++ b/lib/private/share/share.php @@ -1703,7 +1703,7 @@ class Share extends \OC\Share\Constants { $fileTarget = null; } - if ($itemTarget === $groupItemTarget) { + if ($itemTarget === $groupItemTarget && (isset($fileSource) && $fileTarget === $groupItemTarget)) { continue; } } -- cgit v1.2.3 From 1371fecf266765e958dbf07a4dd476c629b32b36 Mon Sep 17 00:00:00 2001 From: Bjoern Schiessle Date: Fri, 26 Sep 2014 16:58:47 +0200 Subject: on unshare only unshare childrens if there is no other parent available --- lib/private/share/helper.php | 24 ++++++++++++++++---- lib/private/share/share.php | 54 +++++++++++++++++++++++++++++++++++--------- 2 files changed, 63 insertions(+), 15 deletions(-) diff --git a/lib/private/share/helper.php b/lib/private/share/helper.php index 7e1cbb273b8..90dd12e9842 100644 --- a/lib/private/share/helper.php +++ b/lib/private/share/helper.php @@ -85,10 +85,12 @@ class Helper extends \OC\Share\Constants { * @param int $parent Id of item to delete * @param bool $excludeParent If true, exclude the parent from the delete (optional) * @param string $uidOwner The user that the parent was shared with (optional) + * @param int $newParent new parent for the childrens */ - public static function delete($parent, $excludeParent = false, $uidOwner = null) { + public static function delete($parent, $excludeParent = false, $uidOwner = null, $newParent = null) { $ids = array($parent); $deletedItems = array(); + $changeParent = array(); $parents = array($parent); while (!empty($parents)) { $parents = "'".implode("','", $parents)."'"; @@ -106,8 +108,6 @@ class Helper extends \OC\Share\Constants { // Reset parents array, only go through loop again if items are found $parents = array(); while ($item = $result->fetchRow()) { - $ids[] = $item['id']; - $parents[] = $item['id']; $tmpItem = array( 'id' => $item['id'], 'shareWith' => $item['share_with'], @@ -118,12 +118,28 @@ class Helper extends \OC\Share\Constants { if (isset($item['file_target'])) { $tmpItem['fileTarget'] = $item['file_target']; } - $deletedItems[] = $tmpItem; + // if we have a new parent for the child we remember the child + // to update the parent, if not we add it to the list of items + // which should be deleted + if ($newParent !== null) { + $changeParent[] = $item['id']; + } else { + $deletedItems[] = $tmpItem; + $ids[] = $item['id']; + $parents[] = $item['id']; + } } } if ($excludeParent) { unset($ids[0]); } + + if (!empty($changeParent)) { + $idList = "'".implode("','", $changeParent)."'"; + $query = \OC_DB::prepare('UPDATE `*PREFIX*share` SET `parent` = ? WHERE `id` IN ('.$idList.')'); + $query->execute(array($newParent)); + } + if (!empty($ids)) { $idList = "'".implode("','", $ids)."'"; $query = \OC_DB::prepare('DELETE FROM `*PREFIX*share` WHERE `id` IN ('.$idList.')'); diff --git a/lib/private/share/share.php b/lib/private/share/share.php index bb0217c0a4c..d6b1a2c6eef 100644 --- a/lib/private/share/share.php +++ b/lib/private/share/share.php @@ -294,23 +294,32 @@ class Share extends \OC\Share\Constants { $shares = array(); + $column = ($itemType === 'file' || $itemType === 'folder') ? 'file_source' : 'item_source'; + + $where = ' `' . $column . '` = ? AND `item_type` = ? '; + $arguments = array($itemSource, $itemType); + // for link shares $user === null + if ($user !== null) { + $where .= ' AND `share_with` = ? '; + $arguments[] = $user; + } + // first check if there is a db entry for the specific user $query = \OC_DB::prepare( - 'SELECT `file_target`, `item_target`, `permissions`, `expiration` + 'SELECT * FROM `*PREFIX*share` - WHERE - `item_source` = ? AND `item_type` = ? AND `share_with` = ?' + WHERE' . $where ); - $result = \OC_DB::executeAudited($query, array($itemSource, $itemType, $user)); + $result = \OC_DB::executeAudited($query, $arguments); while ($row = $result->fetchRow()) { $shares[] = $row; } //if didn't found a result than let's look for a group share. - if(empty($shares)) { + if(empty($shares) && $user !== null) { $groups = \OC_Group::getUserGroups($user); $query = \OC_DB::prepare( @@ -318,7 +327,7 @@ class Share extends \OC\Share\Constants { FROM `*PREFIX*share` WHERE - `item_source` = ? AND `item_type` = ? AND `share_with` in (?)' + `' . $column . '` = ? AND `item_type` = ? AND `share_with` in (?)' ); $result = \OC_DB::executeAudited($query, array($itemSource, $itemType, implode(',', $groups))); @@ -678,9 +687,31 @@ class Share extends \OC\Share\Constants { * @return boolean true on success or false on failure */ public static function unshare($itemType, $itemSource, $shareType, $shareWith) { - $item = self::getItems($itemType, $itemSource, $shareType, $shareWith, \OC_User::getUser(),self::FORMAT_NONE, null, 1); - if (!empty($item)) { - self::unshareItem($item); + + // check if it is a valid itemType + self::getBackend($itemType); + + $items = self::getItemSharedWithUser($itemType, $itemSource, $shareWith); + + $toDelete = array(); + $newParent = null; + $currentUser = \OC_User::getUser(); + foreach ($items as $item) { + // delete the item with the expected share_type and owner + if ((int)$item['share_type'] === (int)$shareType && $item['uid_owner'] === $currentUser) { + $toDelete = $item; + // if there is more then one result we don't have to delete the children + // but update their parent. For group shares the new parent should always be + // the original group share and not the db entry with the unique name + } else if ((int)$item['share_type'] === \OCP\Share::$shareTypeGroupUserUnique) { + $newParent = $item['parent']; + } else { + $newParent = $item['id']; + } + } + + if (!empty($toDelete)) { + self::unshareItem($toDelete, $newParent); return true; } return false; @@ -1052,9 +1083,10 @@ class Share extends \OC\Share\Constants { /** * Unshares a share given a share data array * @param array $item Share data (usually database row) + * @param int new parent ID * @return null */ - protected static function unshareItem(array $item) { + protected static function unshareItem(array $item, $newParent = null) { // Pass all the vars we have for now, they may be useful $hookParams = array( 'id' => $item['id'], @@ -1071,7 +1103,7 @@ class Share extends \OC\Share\Constants { } \OC_Hook::emit('OCP\Share', 'pre_unshare', $hookParams); - $deletedShares = Helper::delete($item['id']); + $deletedShares = Helper::delete($item['id'], false, null, $newParent); $deletedShares[] = $hookParams; $hookParams['deletedShares'] = $deletedShares; \OC_Hook::emit('OCP\Share', 'post_unshare', $hookParams); -- cgit v1.2.3 From bb703006da0ac60abefaedbaf3b61245cc07d4c4 Mon Sep 17 00:00:00 2001 From: Bjoern Schiessle Date: Mon, 29 Sep 2014 11:13:01 +0200 Subject: throw a exception if we can't handle the provided path --- apps/files_sharing/appinfo/app.php | 3 +++ apps/files_sharing/lib/exceptions.php | 32 +++++++++++++++++++++++++++ apps/files_sharing/lib/sharedmount.php | 6 +++--- apps/files_sharing/tests/sharedmount.php | 37 ++++++++++++++++++++++++++++++++ 4 files changed, 75 insertions(+), 3 deletions(-) create mode 100644 apps/files_sharing/lib/exceptions.php diff --git a/apps/files_sharing/appinfo/app.php b/apps/files_sharing/appinfo/app.php index b22c553ec93..ec429529887 100644 --- a/apps/files_sharing/appinfo/app.php +++ b/apps/files_sharing/appinfo/app.php @@ -12,6 +12,9 @@ OC::$CLASSPATH['OCA\Files\Share\Api'] = 'files_sharing/lib/api.php'; OC::$CLASSPATH['OCA\Files\Share\Maintainer'] = 'files_sharing/lib/maintainer.php'; OC::$CLASSPATH['OCA\Files\Share\Proxy'] = 'files_sharing/lib/proxy.php'; +// Exceptions +OC::$CLASSPATH['OCA\Files_Sharing\Exceptions\BrokenPath'] = 'files_sharing/lib/exceptions.php'; + \OCP\App::registerAdmin('files_sharing', 'settings-admin'); \OCA\Files_Sharing\Helper::registerHooks(); diff --git a/apps/files_sharing/lib/exceptions.php b/apps/files_sharing/lib/exceptions.php new file mode 100644 index 00000000000..2a57a69a95f --- /dev/null +++ b/apps/files_sharing/lib/exceptions.php @@ -0,0 +1,32 @@ + + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU AFFERO GENERAL PUBLIC LICENSE + * License as published by the Free Software Foundation; either + * version 3 of the License, or any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU AFFERO GENERAL PUBLIC LICENSE for more details. + * + * You should have received a copy of the GNU Affero General Public + * License along with this library. If not, see . + * + */ + +namespace OCA\Files_Sharing\Exceptions; + +/** + * Expected path with a different root + * Possible Error Codes: + * 10 - Path not relative to data/ and point to the users file directory + + */ +class BrokenPath extends \Exception { +} diff --git a/apps/files_sharing/lib/sharedmount.php b/apps/files_sharing/lib/sharedmount.php index 1717a4bd636..4ad2d4e6b3e 100644 --- a/apps/files_sharing/lib/sharedmount.php +++ b/apps/files_sharing/lib/sharedmount.php @@ -91,7 +91,7 @@ class SharedMount extends Mount implements MoveableMount { * @param string $path the absolute path * @return string e.g. turns '/admin/files/test.txt' into '/test.txt' */ - private function stripUserFilesPath($path) { + protected function stripUserFilesPath($path) { $trimmed = ltrim($path, '/'); $split = explode('/', $trimmed); @@ -99,8 +99,8 @@ class SharedMount extends Mount implements MoveableMount { if (count($split) < 3 || $split[1] !== 'files') { \OCP\Util::writeLog('file sharing', 'Can not strip userid and "files/" from path: ' . $path, - \OCP\Util::DEBUG); - return false; + \OCP\Util::ERROR); + throw new \OCA\Files_Sharing\Exceptions\BrokenPath('Path does not start with /user/files', 10); } // skip 'user' and 'files' diff --git a/apps/files_sharing/tests/sharedmount.php b/apps/files_sharing/tests/sharedmount.php index f8c65734184..ac910944f9f 100644 --- a/apps/files_sharing/tests/sharedmount.php +++ b/apps/files_sharing/tests/sharedmount.php @@ -194,4 +194,41 @@ class Test_Files_Sharing_Mount extends Test_Files_Sharing_Base { \OC_Group::removeFromGroup(self::TEST_FILES_SHARING_API_USER3, 'testGroup'); } + /** + * @dataProvider dataProviderTestStripUserFilesPath + * @param string $path + * @param string $expectedResult + * @param bool $exception if a exception is expected + */ + function testStripUserFilesPath($path, $expectedResult, $exception) { + $testClass = new DummyTestClassSharedMount(null, null); + try { + $result = $testClass->stripUserFilesPathDummy($path); + $this->assertSame($expectedResult, $result); + } catch (\Exception $e) { + if ($exception) { + $this->assertSame(10, $e->getCode()); + } else { + $this->assertTrue(false, "Exception catched, but expected: " . $expectedResult); + } + } + } + + function dataProviderTestStripUserFilesPath() { + return array( + array('/user/files/foo.txt', '/foo.txt', false), + array('/user/files/folder/foo.txt', '/folder/foo.txt', false), + array('/data/user/files/foo.txt', null, true), + array('/data/user/files/', null, true), + array('/files/foo.txt', null, true), + array('/foo.txt', null, true), + ); + } + +} + +class DummyTestClassSharedMount extends \OCA\Files_Sharing\SharedMount { + public function stripUserFilesPathDummy($path) { + return $this->stripUserFilesPath($path); + } } -- cgit v1.2.3 From 92540737861a523593d69a4d90c2aa582d795f7a Mon Sep 17 00:00:00 2001 From: Bjoern Schiessle Date: Mon, 29 Sep 2014 11:23:18 +0200 Subject: some small fixed, suggested by scrutinizer --- apps/files_sharing/lib/share/file.php | 6 ++++-- apps/files_sharing/tests/share.php | 2 -- lib/private/share/helper.php | 4 +--- lib/private/share/hooks.php | 2 +- lib/private/share/share.php | 3 ++- 5 files changed, 8 insertions(+), 9 deletions(-) diff --git a/apps/files_sharing/lib/share/file.php b/apps/files_sharing/lib/share/file.php index 1086e21d79f..dacdb9308be 100644 --- a/apps/files_sharing/lib/share/file.php +++ b/apps/files_sharing/lib/share/file.php @@ -57,7 +57,7 @@ class OC_Share_Backend_File implements OCP\Share_Backend_File_Dependent { * create unique target * @param string $filePath * @param string $shareWith - * @param string $exclude + * @param array $exclude (optional) * @return string */ public function generateTarget($filePath, $shareWith, $exclude = null) { @@ -83,7 +83,9 @@ class OC_Share_Backend_File implements OCP\Share_Backend_File_Dependent { } } - return \OCA\Files_Sharing\Helper::generateUniqueTarget($target, $exclude, $view); + $excludeList = (is_array($exclude)) ? $exclude : array(); + + return \OCA\Files_Sharing\Helper::generateUniqueTarget($target, $excludeList, $view); } public function formatItems($items, $format, $parameters = null) { diff --git a/apps/files_sharing/tests/share.php b/apps/files_sharing/tests/share.php index 80199505d89..fe80cfca781 100644 --- a/apps/files_sharing/tests/share.php +++ b/apps/files_sharing/tests/share.php @@ -72,8 +72,6 @@ class Test_Files_Sharing extends Test_Files_Sharing_Base { $fileinfo = $this->view->getFileInfo($this->filename); - $pathinfo = pathinfo($this->filename); - $result = \OCP\Share::shareItem('file', $fileinfo['fileid'], \OCP\Share::SHARE_TYPE_USER, \Test_Files_Sharing::TEST_FILES_SHARING_API_USER2, 31); diff --git a/lib/private/share/helper.php b/lib/private/share/helper.php index 90dd12e9842..2418535c9d5 100644 --- a/lib/private/share/helper.php +++ b/lib/private/share/helper.php @@ -36,6 +36,7 @@ class Helper extends \OC\Share\Constants { * @return string Item target */ public static function generateTarget($itemType, $itemSource, $shareType, $shareWith, $uidOwner, $suggestedTarget = null, $groupParent = null) { + // FIXME: $uidOwner and $groupParent seems to be unused $backend = \OC\Share\Share::getBackend($itemType); if ($shareType == self::SHARE_TYPE_LINK) { if (isset($suggestedTarget)) { @@ -53,9 +54,6 @@ class Helper extends \OC\Share\Constants { if ($shareType == self::SHARE_TYPE_USER) { // Share with is a user, so set share type to user and groups $shareType = self::$shareTypeUserAndGroups; - $userAndGroups = array_merge(array($shareWith), \OC_Group::getUserGroups($shareWith)); - } else { - $userAndGroups = false; } $exclude = array(); diff --git a/lib/private/share/hooks.php b/lib/private/share/hooks.php index 2be7dc85e3c..66b197b921b 100644 --- a/lib/private/share/hooks.php +++ b/lib/private/share/hooks.php @@ -65,7 +65,7 @@ class Hooks extends \OC\Share\Constants { // do we also need a file target if ($item['item_type'] === 'file' || $item['item_type'] === 'folder') { - $fileTarget = Helper::generateTarget('file', $row['file_target'], self::SHARE_TYPE_USER, $arguments['uid'], + $fileTarget = Helper::generateTarget('file', $item['file_target'], self::SHARE_TYPE_USER, $arguments['uid'], $item['owner'], null, $item['parent']); } else { $fileTarget = null; diff --git a/lib/private/share/share.php b/lib/private/share/share.php index d6b1a2c6eef..cfdc8aa9ffe 100644 --- a/lib/private/share/share.php +++ b/lib/private/share/share.php @@ -703,7 +703,7 @@ class Share extends \OC\Share\Constants { // if there is more then one result we don't have to delete the children // but update their parent. For group shares the new parent should always be // the original group share and not the db entry with the unique name - } else if ((int)$item['share_type'] === \OCP\Share::$shareTypeGroupUserUnique) { + } else if ((int)$item['share_type'] === self::$shareTypeGroupUserUnique) { $newParent = $item['parent']; } else { $newParent = $item['id']; @@ -1610,6 +1610,7 @@ class Share extends \OC\Share\Constants { $permissions, $parentFolder = null, $token = null, $itemSourceName = null, \DateTime $expirationDate = null) { $queriesToExecute = array(); + $suggestedItemTarget = null; $result = self::checkReshare($itemType, $itemSource, $shareType, $shareWith, $uidOwner, $permissions, $itemSourceName, $expirationDate); if(!empty($result)) { -- cgit v1.2.3