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.

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() {