From a238620e16c6c700b9a0805152c18b5acc669543 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Fri, 30 Jan 2026 16:18:24 +0100 Subject: [PATCH] fix: don't rely on share providers being avaiable in CleanupShareTarget Signed-off-by: Robin Appelman --- .../lib/Repair/CleanupShareTarget.php | 47 ++++++++++++------- .../lib/ShareTargetValidator.php | 12 ++--- 2 files changed, 36 insertions(+), 23 deletions(-) diff --git a/apps/files_sharing/lib/Repair/CleanupShareTarget.php b/apps/files_sharing/lib/Repair/CleanupShareTarget.php index 6a97bc4891338..9304c7981cea1 100644 --- a/apps/files_sharing/lib/Repair/CleanupShareTarget.php +++ b/apps/files_sharing/lib/Repair/CleanupShareTarget.php @@ -13,14 +13,16 @@ use OCP\DB\QueryBuilder\IQueryBuilder; use OCP\Files\Config\ICachedMountInfo; use OCP\Files\Config\IUserMountCache; +use OCP\Files\IRootFolder; 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; +/** + * @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 @@ -34,12 +36,11 @@ 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 IUserMountCache $userMountCache, + private readonly IRootFolder $rootFolder, ) { } @@ -58,12 +59,10 @@ 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()); // since we ordered the share by user, we can reuse the last data until we get to the next user if ($lastUser !== $recipient->getUID()) { @@ -74,19 +73,23 @@ public function run(IOutput $output) { $mounts = $this->userMountCache->getMountsForUser($recipient); $mountPoints = array_map(fn (ICachedMountInfo $mount) => $mount->getMountPoint(), $mounts); $userMounts = array_combine($mountPoints, $mounts); + $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()); + $absoluteNewTarget = $userFolder->getFullPath($newTarget); + $targetParentNode = $this->rootFolder->get(dirname($absoluteNewTarget)); - $this->shareTargetValidator->verifyMountPoint( - $recipient, - $share, + $absoluteNewTarget = $this->shareTargetValidator->generateUniqueTarget( + (int)$shareInfo['file_source'], + $absoluteNewTarget, + $targetParentNode->getMountPoint(), $userMounts, - [$share], ); + $newTarget = $userFolder->getRelativePath($absoluteNewTarget); + + $this->moveShare((string)$shareInfo['id'], $newTarget); $oldMountPoint = "/{$recipient->getUID()}/files$oldTarget/"; $newMountPoint = "/{$recipient->getUID()}/files$newTarget/"; @@ -108,19 +111,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 + * @return \Traversable */ 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 $rows */ + /** @var \Traversable $rows */ $rows = $result->iterateAssociative(); return $rows; } diff --git a/apps/files_sharing/lib/ShareTargetValidator.php b/apps/files_sharing/lib/ShareTargetValidator.php index b47dde1138112..554f76c0dca41 100644 --- a/apps/files_sharing/lib/ShareTargetValidator.php +++ b/apps/files_sharing/lib/ShareTargetValidator.php @@ -85,7 +85,7 @@ public function verifyMountPoint( } $newAbsoluteMountPoint = $this->generateUniqueTarget( - $share, + $share->getNodeId(), Filesystem::normalizePath($absoluteParent . '/' . $mountPoint), $parentMount, $allCachedMounts, @@ -108,8 +108,8 @@ public function verifyMountPoint( /** * @param ICachedMountInfo[] $allCachedMounts */ - private function generateUniqueTarget( - IShare $share, + public function generateUniqueTarget( + int $shareNodeId, string $absolutePath, IMountPoint $parentMount, array $allCachedMounts, @@ -122,7 +122,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++; @@ -134,13 +134,13 @@ private function generateUniqueTarget( /** * @param ICachedMountInfo[] $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->getMountProvider() === MountProvider::class && $mount->getRootId() === $share->getNodeId()) { + if ($mount->getMountProvider() === MountProvider::class && $mount->getRootId() === $shareNodeId) { // "conflicting" mount is a mount for the current share return false; }