From 9f137ac25991da89425f6140dc5e078bd0f7d21d Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Tue, 20 Jan 2015 13:09:39 +0100 Subject: [PATCH] Fix reshare permission change to not impair other deletion code A recent change that prevents reshare permission changes to delete group share children had the side-effect of also preventing group share children deletion when it needed to be done. This fix adds an extra flag to isolate the "reshare permission change" deletion case and keep the other ones as they were before, not only to fix the regression but also fix other potential regressions in code that uses this method. Also updated the comment because now Helper::delete() is no longer limited to reshares but also applies to group share children. --- lib/private/share/helper.php | 27 +++++++++++++++------ lib/private/share/share.php | 3 ++- tests/lib/share/share.php | 47 ++++++++++++++++++++++++++++++++++++ 3 files changed, 68 insertions(+), 9 deletions(-) diff --git a/lib/private/share/helper.php b/lib/private/share/helper.php index 5b27f0e6f50..6059af0196d 100644 --- a/lib/private/share/helper.php +++ b/lib/private/share/helper.php @@ -79,13 +79,14 @@ class Helper extends \OC\Share\Constants { } /** - * Delete all reshares of an item + * Delete all reshares and group share children of an item * @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 + * @param bool $excludeGroupChildren exclude group children elements */ - public static function delete($parent, $excludeParent = false, $uidOwner = null, $newParent = null) { + public static function delete($parent, $excludeParent = false, $uidOwner = null, $newParent = null, $excludeGroupChildren = false) { $ids = array($parent); $deletedItems = array(); $changeParent = array(); @@ -94,15 +95,25 @@ class Helper extends \OC\Share\Constants { $parents = "'".implode("','", $parents)."'"; // Check the owner on the first search of reshares, useful for // finding and deleting the reshares by a single user of a group share + $params = array(); if (count($ids) == 1 && isset($uidOwner)) { - $query = \OC_DB::prepare('SELECT `id`, `share_with`, `item_type`, `share_type`, `item_target`, `file_target`, `parent`' - .' FROM `*PREFIX*share` WHERE `parent` IN ('.$parents.') AND `uid_owner` = ? AND `share_type` != ?'); - $result = $query->execute(array($uidOwner, self::$shareTypeGroupUserUnique)); + // FIXME: don't concat $parents, use Docrine's PARAM_INT_ARRAY approach + $queryString = 'SELECT `id`, `share_with`, `item_type`, `share_type`, ' . + '`item_target`, `file_target`, `parent` ' . + 'FROM `*PREFIX*share` ' . + 'WHERE `parent` IN ('.$parents.') AND `uid_owner` = ? '; + $params[] = $uidOwner; } else { - $query = \OC_DB::prepare('SELECT `id`, `share_with`, `item_type`, `share_type`, `item_target`, `file_target`, `parent`, `uid_owner`' - .' FROM `*PREFIX*share` WHERE `parent` IN ('.$parents.') AND `share_type` != ?'); - $result = $query->execute(array(self::$shareTypeGroupUserUnique)); + $queryString = 'SELECT `id`, `share_with`, `item_type`, `share_type`, ' . + '`item_target`, `file_target`, `parent`, `uid_owner` ' . + 'FROM `*PREFIX*share` WHERE `parent` IN ('.$parents.') '; } + if ($excludeGroupChildren) { + $queryString .= ' AND `share_type` != ?'; + $params[] = self::$shareTypeGroupUserUnique; + } + $query = \OC_DB::prepare($queryString); + $result = $query->execute($params); // Reset parents array, only go through loop again if items are found $parents = array(); while ($item = $result->fetchRow()) { diff --git a/lib/private/share/share.php b/lib/private/share/share.php index e85f9f06ed3..e5f350a24fb 100644 --- a/lib/private/share/share.php +++ b/lib/private/share/share.php @@ -971,7 +971,8 @@ class Share extends \OC\Share\Constants { if ($item['permissions'] & ~$permissions) { // If share permission is removed all reshares must be deleted if (($item['permissions'] & \OCP\Constants::PERMISSION_SHARE) && (~$permissions & \OCP\Constants::PERMISSION_SHARE)) { - Helper::delete($item['id'], true); + // delete all shares, keep parent and group children + Helper::delete($item['id'], true, null, null, true); } else { $ids = array(); $parents = array($item['id']); diff --git a/tests/lib/share/share.php b/tests/lib/share/share.php index b1261b0afbd..4b42036fc22 100644 --- a/tests/lib/share/share.php +++ b/tests/lib/share/share.php @@ -44,11 +44,13 @@ class Test_Share extends \Test\TestCase { $this->user2 = $this->getUniqueID('user2_'); $this->user3 = $this->getUniqueID('user3_'); $this->user4 = $this->getUniqueID('user4_'); + $this->user5 = $this->getUniqueID('user5_'); $this->groupAndUser = $this->getUniqueID('groupAndUser_'); OC_User::createUser($this->user1, 'pass'); OC_User::createUser($this->user2, 'pass'); OC_User::createUser($this->user3, 'pass'); OC_User::createUser($this->user4, 'pass'); + OC_User::createUser($this->user5, 'pass'); OC_User::createUser($this->groupAndUser, 'pass'); OC_User::setUserId($this->user1); OC_Group::clearBackends(); @@ -610,6 +612,51 @@ class Test_Share extends \Test\TestCase { $this->assertEquals(array(), OCP\Share::getItemsShared('test')); } + /** + * Test that unsharing from group will also delete all + * child entries + */ + public function testShareWithGroupThenUnshare() { + OC_User::setUserId($this->user5); + OCP\Share::shareItem( + 'test', + 'test.txt', + OCP\Share::SHARE_TYPE_GROUP, + $this->group1, + \OCP\Constants::PERMISSION_ALL + ); + + $targetUsers = array($this->user1, $this->user2, $this->user3); + + foreach($targetUsers as $targetUser) { + OC_User::setUserId($targetUser); + $items = OCP\Share::getItemsSharedWithUser( + 'test', + $targetUser, + Test_Share_Backend::FORMAT_TARGET + ); + $this->assertEquals(1, count($items)); + } + + OC_User::setUserId($this->user5); + OCP\Share::unshare( + 'test', + 'test.txt', + OCP\Share::SHARE_TYPE_GROUP, + $this->group1 + ); + + // verify that all were deleted + foreach($targetUsers as $targetUser) { + OC_User::setUserId($targetUser); + $items = OCP\Share::getItemsSharedWithUser( + 'test', + $targetUser, + Test_Share_Backend::FORMAT_TARGET + ); + $this->assertEquals(0, count($items)); + } + } public function testShareWithGroupAndUserBothHaveTheSameId() { -- 2.39.5