generateTarget() will always find a unique target

lib/private/share/helper.php

@@ -35,8 +35,7 @@ class Helper extends \OC\Share\Constants {
 	 * @throws \Exception
 	 * @return string Item target
 	 */
-	public static function generateTarget($itemType, $itemSource, $shareType, $shareWith, $uidOwner,
-		$suggestedTarget = null, $groupParent = null) {
+	public static function generateTarget($itemType, $itemSource, $shareType, $shareWith, $uidOwner, $suggestedTarget = null, $groupParent = null) {
 		$backend = \OC\Share\Share::getBackend($itemType);
 		if ($shareType == self::SHARE_TYPE_LINK) {
 			if (isset($suggestedTarget)) {
@@ -58,87 +57,51 @@ class Helper extends \OC\Share\Constants {
 			} else {
 				$userAndGroups = false;
 			}
-			$exclude = null;
-			// Backend has 3 opportunities to generate a unique target
-			for ($i = 0; $i < 2; $i++) {
-				// Check if suggested target exists first
-				if ($i == 0 && isset($suggestedTarget)) {
-					$target = $suggestedTarget;
+			$exclude = array();
+			// Find similar targets to improve backend's chances to generate a unqiue target
+			if ($userAndGroups) {
+				if ($column == 'file_target') {
+					$checkTargets = \OC_DB::prepare('SELECT `' . $column . '` FROM `*PREFIX*share`'
+									. ' WHERE `item_type` IN (\'file\', \'folder\')'
+									. ' AND `share_type` IN (?,?,?)'
+									. ' AND `share_with` IN (\'' . implode('\',\'', $userAndGroups) . '\')');
+					$result = $checkTargets->execute(array(self::SHARE_TYPE_USER, self::SHARE_TYPE_GROUP,
+						self::$shareTypeGroupUserUnique));
 				} else {
-					if ($shareType == self::SHARE_TYPE_GROUP) {
-						$target = $backend->generateTarget($itemSource, false, $exclude);
-					} else {
-						$target = $backend->generateTarget($itemSource, $shareWith, $exclude);
-					}
-					if (is_array($exclude) && in_array($target, $exclude)) {
-						break;
-					}
+					$checkTargets = \OC_DB::prepare('SELECT `' . $column . '` FROM `*PREFIX*share`'
+									. ' WHERE `item_type` = ? AND `share_type` IN (?,?,?)'
+									. ' AND `share_with` IN (\'' . implode('\',\'', $userAndGroups) . '\')');
+					$result = $checkTargets->execute(array($itemType, self::SHARE_TYPE_USER,
+						self::SHARE_TYPE_GROUP, self::$shareTypeGroupUserUnique));
 				}
-				// Check if target already exists
-				$checkTarget = \OC\Share\Share::getItems($itemType, $target, $shareType, $shareWith);
-				if (!empty($checkTarget)) {
-					foreach ($checkTarget as $item) {
-						// Skip item if it is the group parent row
-						if (isset($groupParent) && $item['id'] == $groupParent) {
-							if (count($checkTarget) == 1) {
-								return $target;
-							} else {
-								continue;
-							}
-						}
-						if ($item['uid_owner'] == $uidOwner) {
-							if ($itemType == 'file' || $itemType == 'folder') {
-								$meta = \OC\Files\Filesystem::getFileInfo($itemSource);
-								if ($item['file_source'] == $meta['fileid']) {
-									return $target;
-								}
-							} else if ($item['item_source'] == $itemSource) {
-								return $target;
-							}
-						}
-					}
-					if (!isset($exclude)) {
-						$exclude = array();
-					}
-					// Find similar targets to improve backend's chances to generate a unqiue target
-					if ($userAndGroups) {
-						if ($column == 'file_target') {
-							$checkTargets = \OC_DB::prepare('SELECT `'.$column.'` FROM `*PREFIX*share`'
-								.' WHERE `item_type` IN (\'file\', \'folder\')'
-								.' AND `share_type` IN (?,?,?)'
-								.' AND `share_with` IN (\''.implode('\',\'', $userAndGroups).'\')');
-							$result = $checkTargets->execute(array(self::SHARE_TYPE_USER, self::SHARE_TYPE_GROUP,
-								self::$shareTypeGroupUserUnique));
-						} else {
-							$checkTargets = \OC_DB::prepare('SELECT `'.$column.'` FROM `*PREFIX*share`'
-								.' WHERE `item_type` = ? AND `share_type` IN (?,?,?)'
-								.' AND `share_with` IN (\''.implode('\',\'', $userAndGroups).'\')');
-							$result = $checkTargets->execute(array($itemType, self::SHARE_TYPE_USER,
-								self::SHARE_TYPE_GROUP, self::$shareTypeGroupUserUnique));
-						}
-					} else {
-						if ($column == 'file_target') {
-							$checkTargets = \OC_DB::prepare('SELECT `'.$column.'` FROM `*PREFIX*share`'
-								.' WHERE `item_type` IN (\'file\', \'folder\')'
-								.' AND `share_type` = ? AND `share_with` = ?');
-							$result = $checkTargets->execute(array(self::SHARE_TYPE_GROUP, $shareWith));
-						} else {
-							$checkTargets = \OC_DB::prepare('SELECT `'.$column.'` FROM `*PREFIX*share`'
-								.' WHERE `item_type` = ? AND `share_type` = ? AND `share_with` = ?');
-							$result = $checkTargets->execute(array($itemType, self::SHARE_TYPE_GROUP, $shareWith));
-						}
-					}
-					while ($row = $result->fetchRow()) {
-						$exclude[] = $row[$column];
-					}
+			} else {
+				if ($column == 'file_target') {
+					$checkTargets = \OC_DB::prepare('SELECT `' . $column . '` FROM `*PREFIX*share`'
+									. ' WHERE `item_type` IN (\'file\', \'folder\')'
+									. ' AND `share_type` = ? AND `share_with` = ?');
+					$result = $checkTargets->execute(array(self::SHARE_TYPE_GROUP, $shareWith));
 				} else {
-					return $target;
+					$checkTargets = \OC_DB::prepare('SELECT `' . $column . '` FROM `*PREFIX*share`'
+									. ' WHERE `item_type` = ? AND `share_type` = ? AND `share_with` = ?');
+					$result = $checkTargets->execute(array($itemType, self::SHARE_TYPE_GROUP, $shareWith));
 				}
 			}
+			while ($row = $result->fetchRow()) {
+				$exclude[] = $row[$column];
+			}
+
+			// Check if suggested target exists first
+			if (!isset($suggestedTarget)) {
+				$suggestedTarget = $itemSource;
+			}
+			if ($shareType == self::SHARE_TYPE_GROUP) {
+				$target = $backend->generateTarget($suggestedTarget, false, $exclude);
+			} else {
+				$target = $backend->generateTarget($suggestedTarget, $shareWith, $exclude);
+			}
+
+			return $target;
 		}
-		$message = 'Sharing backend registered for '.$itemType.' did not generate a unique target for '.$itemSource;
-		\OC_Log::write('OCP\Share', $message, \OC_Log::ERROR);
-		throw new \Exception($message);
 	}
 
 	/**

lib/private/share/share.php

@@ -296,7 +296,7 @@ class Share extends \OC\Share\Constants {
 
 		// first check if there is a db entry for the specific user
 		$query = \OC_DB::prepare(
-				'SELECT `file_target`, `permissions`, `expiration`
+				'SELECT `file_target`, `item_target`, `permissions`, `expiration`
 					FROM
 					`*PREFIX*share`
 					WHERE
@@ -1294,13 +1294,17 @@ class Share extends \OC\Share\Constants {
 				$queryArgs = array_merge($queryArgs, $collectionTypes);
 			}
 		}
+
+		if ($shareType == self::$shareTypeUserAndGroups) {
+			// Make sure the unique user target is returned if it exists,
+			// unique targets should follow the group share in the database
+			// If the limit is not 1, the filtering can be done later
+			$where .= ' ORDER BY `*PREFIX*share`.`id` DESC';
+		} else {
+			$where .= ' ORDER BY `*PREFIX*share`.`id` ASC';
+		}
+
 		if ($limit != -1 && !$includeCollections) {
-			if ($shareType == self::$shareTypeUserAndGroups) {
-				// Make sure the unique user target is returned if it exists,
-				// unique targets should follow the group share in the database
-				// If the limit is not 1, the filtering can be done later
-				$where .= ' ORDER BY `*PREFIX*share`.`id` ASC';
-			}
 			// The limit must be at least 3, because filtering needs to be done
 			if ($limit < 3) {
 				$queryLimit = 3;
@@ -1309,7 +1313,6 @@ class Share extends \OC\Share\Constants {
 			}
 		} else {
 			$queryLimit = null;
-			$where .= ' ORDER BY `*PREFIX*share`.`id` ASC';
 		}
 		$select = self::createSelectStatement($format, $fileDependent, $uidOwner);
 		$root = strlen($root);
@@ -1338,10 +1341,10 @@ class Share extends \OC\Share\Constants {
 				}
 			} else if (!isset($uidOwner)) {
 				// Check if the same target already exists
-				if (isset($targets[$row[$column]])) {
+				if (isset($targets[$row['id']])) {
 					// Check if the same owner shared with the user twice
 					// through a group and user share - this is allowed
-					$id = $targets[$row[$column]];
+					$id = $targets[$row['id']];
 					if (isset($items[$id]) && $items[$id]['uid_owner'] == $row['uid_owner']) {
 						// Switch to group share type to ensure resharing conditions aren't bypassed
 						if ($items[$id]['share_type'] != self::SHARE_TYPE_GROUP) {
@@ -1357,12 +1360,12 @@ class Share extends \OC\Share\Constants {
 							unset($items[$id]);
 							$id = $row['id'];
 						}
-						// Combine the permissions for the item
 						$items[$id]['permissions'] |= (int)$row['permissions'];
-						continue;
+
 					}
-				} else {
-					$targets[$row[$column]] = $row['id'];
+					continue;
+				} elseif (!empty($row['parent'])) {
+					$targets[$row['parent']] = $row['id'];
 				}
 			}
 			// Remove root from file source paths if retrieving own shared items
@@ -1417,10 +1420,13 @@ class Share extends \OC\Share\Constants {
 				$row['displayname_owner'] = \OCP\User::getDisplayName($row['uid_owner']);
 			}
 
-			$items[$row['id']] = $row;
+			if ($row['permissions'] > 0) {
+				$items[$row['id']] = $row;
+			}
 
 		}
 
+		// group items if we are looking for items shared with the current user
 		if (isset($shareWith) && $shareWith === \OCP\User::getUser()) {
 			$items = self::groupItems($items, $itemType);
 		}
@@ -1646,7 +1652,8 @@ class Share extends \OC\Share\Constants {
 		}
 
 		foreach ($users as $user) {
-			$sourceExists = self::getItemSharedWithBySource($itemType, $itemSource, self::FORMAT_NONE, null, true, $user);
+			$sourceId = ($itemType === 'file' || $itemType === 'folder') ? $fileSource : $itemSource;
+			$sourceExists = self::getItemSharedWithBySource($itemType, $sourceId, self::FORMAT_NONE, null, true, $user);
 
 			$shareType = ($isGroupShare) ? self::$shareTypeGroupUserUnique : $shareType;
 
@@ -1679,9 +1686,23 @@ class Share extends \OC\Share\Constants {
 				}
 
 			} else {
-                // move to the next user if it is neither a unique group share
-				// nor a single share without adding it to the share table
-				continue;
+
+				// group share which doesn't exists until now, check if we need a unique target for this user
+
+				$itemTarget = Helper::generateTarget($itemType, $itemSource, self::SHARE_TYPE_USER, $user,
+					$uidOwner, $suggestedItemTarget, $parent);
+
+				// do we also need a file target
+				if (isset($fileSource)) {
+					$fileTarget = Helper::generateTarget('file', $filePath, self::SHARE_TYPE_USER, $user,
+							$uidOwner, $suggestedFileTarget, $parent);
+				} else {
+					$fileTarget = null;
+				}
+
+				if ($itemTarget === $groupItemTarget) {
+					continue;
+				}
 			}
 
 			$queriesToExecute[] = array(