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.

diff --git a/lib/private/share/helper.php b/lib/private/share/helper.php
index 5b27f0e6f500690066e0ea01727663ceb98641bb..6059af0196d9ea152978856cfa386ffec77cbb15 100644 (file)
--- 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 e85f9f06ed3749a48647813d96d37d41fc786277..e5f350a24fbc0f31296db67c9ca5e2f9bcec558b 100644 (file)
--- 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 b1261b0afbdff7aaa63716282f3cbc86da760678..4b42036fc222dcf208532248f1617b34156880bf 100644 (file)
--- 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() {