Merge pull request #57927 from nextcloud/share-target-repair-no-providers-33

[stable33] fix: don't rely on share providers being avaiable in CleanupShareTarget

Author: Altahrim

apps/files_sharing/lib/Repair/CleanupShareTarget.php

@@ -11,15 +11,18 @@
 use OC\Files\SetupManager;
 use OCA\Files_Sharing\ShareTargetValidator;
 use OCP\DB\QueryBuilder\IQueryBuilder;
+use OCP\Files\IRootFolder;
 use OCP\Files\Mount\IMountManager;
 use OCP\IDBConnection;
 use OCP\IUserManager;
 use OCP\Migration\IOutput;
 use OCP\Migration\IRepairStep;
-use OCP\Share\IManager;
-use OCP\Share\IProviderFactory;
 use OCP\Share\IShare;
+use Psr\Log\LoggerInterface;
 
+/**
+ * @psalm-type ShareInfo = array{id: string|int, share_type: string, share_with: string, file_source: string, file_target: string}
+ */
 class CleanupShareTarget implements IRepairStep {
 	/** we only care about shares with a user target,
 	 *  since the underling group/deck/talk share doesn't get moved
@@ -33,12 +36,12 @@ class CleanupShareTarget implements IRepairStep {
 
 	public function __construct(
 		private readonly IDBConnection $connection,
-		private readonly IManager $shareManager,
-		private readonly IProviderFactory $shareProviderFactory,
 		private readonly ShareTargetValidator $shareTargetValidator,
 		private readonly IUserManager $userManager,
 		private readonly SetupManager $setupManager,
 		private readonly IMountManager $mountManager,
+		private readonly IRootFolder $rootFolder,
+		private readonly LoggerInterface $logger,
 	) {
 	}
 
@@ -57,12 +60,13 @@ public function run(IOutput $output) {
 
 		$lastUser = '';
 		$userMounts = [];
+		$userFolder = null;
 
 		foreach ($this->getProblemShares() as $shareInfo) {
 			$recipient = $this->userManager->getExistingUser($shareInfo['share_with']);
-			$share = $this->shareProviderFactory
-				->getProviderForType((int)$shareInfo['share_type'])
-				->getShareById($shareInfo['id'], $recipient->getUID());
+			if (!$recipient->isEnabled()) {
+				continue;
+			}
 
 			// since we ordered the share by user, we can reuse the last data until we get to the next user
 			if ($lastUser !== $recipient->getUID()) {
@@ -71,24 +75,34 @@ public function run(IOutput $output) {
 				$this->setupManager->tearDown();
 				$this->setupManager->setupForUser($recipient);
 				$userMounts = $this->mountManager->getAll();
+				$userFolder = $this->rootFolder->getUserFolder($recipient->getUID());
 			}
 
-			$oldTarget = $share->getTarget();
+			$oldTarget = $shareInfo['file_target'];
 			$newTarget = $this->cleanTarget($oldTarget);
-			$share->setTarget($newTarget);
-			$this->shareManager->moveShare($share, $recipient->getUID());
-
-			$this->shareTargetValidator->verifyMountPoint(
-				$recipient,
-				$share,
-				$userMounts,
-				[$share],
-			);
-
-			$oldMountPoint = "/{$recipient->getUID()}/files$oldTarget/";
-			$newMountPoint = "/{$recipient->getUID()}/files$newTarget/";
-			$userMounts[$newMountPoint] = $userMounts[$oldMountPoint];
-			unset($userMounts[$oldMountPoint]);
+			$absoluteNewTarget = $userFolder->getFullPath($newTarget);
+			$targetParentNode = $this->rootFolder->get(dirname($absoluteNewTarget));
+
+			try {
+				$absoluteNewTarget = $this->shareTargetValidator->generateUniqueTarget(
+					(int)$shareInfo['file_source'],
+					$absoluteNewTarget,
+					$targetParentNode->getMountPoint(),
+					$userMounts,
+				);
+				$newTarget = $userFolder->getRelativePath($absoluteNewTarget);
+
+				$this->moveShare((string)$shareInfo['id'], $newTarget);
+
+				$oldMountPoint = "/{$recipient->getUID()}/files$oldTarget/";
+				$newMountPoint = "/{$recipient->getUID()}/files$newTarget/";
+				$userMounts[$newMountPoint] = $userMounts[$oldMountPoint];
+				unset($userMounts[$oldMountPoint]);
+			} catch (\Exception $e) {
+				$msg = 'error cleaning up share target: ' . $e->getMessage();
+				$this->logger->error($msg, ['exception' => $e, 'app' => 'files_sharing']);
+				$output->warning($msg);
+			}
 
 			$output->advance();
 		}
@@ -105,19 +119,29 @@ private function countProblemShares(): int {
 		return (int)$query->executeQuery()->fetchOne();
 	}
 
+	private function moveShare(string $id, string $target) {
+		// since we only process user-specific shares, we can just move them
+		// without having to check if we need to create a user-specific override
+		$query = $this->connection->getQueryBuilder();
+		$query->update('share')
+			->set('file_target', $query->createNamedParameter($target))
+			->where($query->expr()->eq('id', $query->createNamedParameter($id)))
+			->executeStatement();
+	}
+
 	/**
-	 * @return \Traversable<array{id: string, share_type: string, share_with: string}>
+	 * @return \Traversable<ShareInfo>
 	 */
 	private function getProblemShares(): \Traversable {
 		$query = $this->connection->getQueryBuilder();
-		$query->select('id', 'share_type', 'share_with')
+		$query->select('id', 'share_type', 'share_with', 'file_source', 'file_target')
 			->from('share')
 			->where($query->expr()->like('file_target', $query->createNamedParameter('% (_) (_)%')))
 			->andWhere($query->expr()->in('share_type', $query->createNamedParameter(self::USER_SHARE_TYPES, IQueryBuilder::PARAM_INT_ARRAY), IQueryBuilder::PARAM_INT_ARRAY))
 			->orderBy('share_with')
 			->addOrderBy('id');
 		$result = $query->executeQuery();
-		/** @var \Traversable<array{id: string, share_type: string, share_with: string}> $rows */
+		/** @var \Traversable<ShareInfo> $rows */
 		$rows = $result->iterateAssociative();
 		return $rows;
 	}

apps/files_sharing/lib/ShareTargetValidator.php

@@ -84,7 +84,7 @@ public function verifyMountPoint(
 		}
 
 		$newAbsoluteMountPoint = $this->generateUniqueTarget(
-			$share,
+			$share->getNodeId(),
 			Filesystem::normalizePath($absoluteParent . '/' . $mountPoint),
 			$parentMount,
 			$allCachedMounts,
@@ -107,8 +107,8 @@ public function verifyMountPoint(
 	/**
 	 * @param IMountPoint[] $allCachedMounts
 	 */
-	private function generateUniqueTarget(
-		IShare $share,
+	public function generateUniqueTarget(
+		int $shareNodeId,
 		string $absolutePath,
 		IMountPoint $parentMount,
 		array $allCachedMounts,
@@ -121,7 +121,7 @@ private function generateUniqueTarget(
 		$i = 2;
 		$parentCache = $parentMount->getStorage()->getCache();
 		$internalPath = $parentMount->getInternalPath($absolutePath);
-		while ($parentCache->inCache($internalPath) || $this->hasConflictingMount($share, $allCachedMounts, $absolutePath)) {
+		while ($parentCache->inCache($internalPath) || $this->hasConflictingMount($shareNodeId, $allCachedMounts, $absolutePath)) {
 			$absolutePath = Filesystem::normalizePath($dir . '/' . $name . ' (' . $i . ')' . $ext);
 			$internalPath = $parentMount->getInternalPath($absolutePath);
 			$i++;
@@ -133,13 +133,13 @@ private function generateUniqueTarget(
 	/**
 	 * @param IMountPoint[] $allCachedMounts
 	 */
-	private function hasConflictingMount(IShare $share, array $allCachedMounts, string $absolutePath): bool {
+	private function hasConflictingMount(int $shareNodeId, array $allCachedMounts, string $absolutePath): bool {
 		if (!isset($allCachedMounts[$absolutePath . '/'])) {
 			return false;
 		}
 
 		$mount = $allCachedMounts[$absolutePath . '/'];
-		if ($mount instanceof SharedMount && $mount->getShare()->getNodeId() === $share->getNodeId()) {
+		if ($mount instanceof SharedMount && $mount->getShare()->getNodeId() === $shareNodeId) {
 			// "conflicting" mount is a mount for the current share
 			return false;
 		}