From 9f137ac25991da89425f6140dc5e078bd0f7d21d Mon Sep 17 00:00:00 2001
From: Vincent Petry <pvince81@owncloud.com>
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 5b27f0e6f5..6059af0196 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 e85f9f06ed..e5f350a24f 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 b1261b0afb..4b42036fc2 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() {
 
-- 
GitLab