Merge pull request #13509 from owncloud/share-deletechildrenwhenunsharefromgroup

Fix reshare permission change to not impair other deletion code

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`'
+				// FIXME: don't concat $parents, use Docrine's PARAM_INT_ARRAY approach
-					.' FROM `*PREFIX*share` WHERE `parent` IN ('.$parents.') AND `uid_owner` = ? AND `share_type` != ?');
+				$queryString = 'SELECT `id`, `share_with`, `item_type`, `share_type`, ' .
-				$result = $query->execute(array($uidOwner, self::$shareTypeGroupUserUnique));
+					'`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`'
+				$queryString = 'SELECT `id`, `share_with`, `item_type`, `share_type`, ' .
-					.' FROM `*PREFIX*share` WHERE `parent` IN ('.$parents.') AND `share_type` != ?');
+					'`item_target`, `file_target`, `parent`, `uid_owner` ' .
-				$result = $query->execute(array(self::$shareTypeGroupUserUnique));
+					'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()) {

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']);

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