Merge pull request #57822 from nextcloud/share-mount-validation-fixes-33

[stable33] fix: improve share mount conflict resolution logic

apps/files_sharing/composer/composer/autoload_classmap.php

@@ -96,6 +96,7 @@ return array(
     'OCA\\Files_Sharing\\Settings\\Personal' => $baseDir . '/../lib/Settings/Personal.php',
     'OCA\\Files_Sharing\\ShareBackend\\File' => $baseDir . '/../lib/ShareBackend/File.php',
     'OCA\\Files_Sharing\\ShareBackend\\Folder' => $baseDir . '/../lib/ShareBackend/Folder.php',
+    'OCA\\Files_Sharing\\ShareTargetValidator' => $baseDir . '/../lib/ShareTargetValidator.php',
     'OCA\\Files_Sharing\\SharedMount' => $baseDir . '/../lib/SharedMount.php',
     'OCA\\Files_Sharing\\SharedStorage' => $baseDir . '/../lib/SharedStorage.php',
     'OCA\\Files_Sharing\\SharesReminderJob' => $baseDir . '/../lib/SharesReminderJob.php',

apps/files_sharing/composer/composer/autoload_static.php

@@ -111,6 +111,7 @@ class ComposerStaticInitFiles_Sharing
         'OCA\\Files_Sharing\\Settings\\Personal' => __DIR__ . '/..' . '/../lib/Settings/Personal.php',
         'OCA\\Files_Sharing\\ShareBackend\\File' => __DIR__ . '/..' . '/../lib/ShareBackend/File.php',
         'OCA\\Files_Sharing\\ShareBackend\\Folder' => __DIR__ . '/..' . '/../lib/ShareBackend/Folder.php',
+        'OCA\\Files_Sharing\\ShareTargetValidator' => __DIR__ . '/..' . '/../lib/ShareTargetValidator.php',
         'OCA\\Files_Sharing\\SharedMount' => __DIR__ . '/..' . '/../lib/SharedMount.php',
         'OCA\\Files_Sharing\\SharedStorage' => __DIR__ . '/..' . '/../lib/SharedStorage.php',
         'OCA\\Files_Sharing\\SharesReminderJob' => __DIR__ . '/..' . '/../lib/SharesReminderJob.php',

apps/files_sharing/lib/MountProvider.php

@@ -11,7 +11,6 @@ use Exception;
 use InvalidArgumentException;
 use OC\Files\View;
 use OCA\Files_Sharing\Event\ShareMountedEvent;
-use OCP\Cache\CappedMemoryCache;
 use OCP\EventDispatcher\IEventDispatcher;
 use OCP\Files\Config\IMountProvider;
 use OCP\Files\Config\IPartialMountProvider;
@@ -29,6 +28,7 @@ use Psr\Log\LoggerInterface;
 use function count;
 
 class MountProvider implements IMountProvider, IPartialMountProvider {
+
 	/**
 	 * @param IConfig $config
 	 * @param IManager $shareManager
@@ -41,6 +41,7 @@ class MountProvider implements IMountProvider, IPartialMountProvider {
 		protected IEventDispatcher $eventDispatcher,
 		protected ICacheFactory $cacheFactory,
 		protected IMountManager $mountManager,
+		protected ShareTargetValidator $shareTargetValidator,
 	) {
 	}
 
@@ -270,8 +271,6 @@ class MountProvider implements IMountProvider, IPartialMountProvider {
 		$view = new View('/' . $userId . '/files');
 		$ownerViews = [];
 		$sharingDisabledForUser = $this->shareManager->sharingDisabledForUser($userId);
-		/** @var CappedMemoryCache<bool> $folderExistCache */
-		$foldersExistCache = new CappedMemoryCache();
 
 		$validShareCache = $this->cacheFactory->createLocal('share-valid-mountpoint-max');
 		$maxValidatedShare = $validShareCache->get($userId) ?? 0;
@@ -292,10 +291,17 @@ class MountProvider implements IMountProvider, IPartialMountProvider {
 				if (!isset($ownerViews[$owner])) {
 					$ownerViews[$owner] = new View('/' . $owner . '/files');
 				}
+
 				$shareId = (int)$parentShare->getId();
+				$absMountPoint = '/' . $user->getUID() . '/files/' . trim($parentShare->getTarget(), '/') . '/';
+
+				// after the mountpoint is verified for the first time, only new mountpoints (e.g. groupfolders can overwrite the target)
+				if ($shareId > $maxValidatedShare || isset($allMounts[$absMountPoint])) {
+					$this->shareTargetValidator->verifyMountPoint($user, $parentShare, $allMounts, $groupedShares);
+				}
+
 				$mount = new SharedMount(
 					'\OCA\Files_Sharing\SharedStorage',
-					$allMounts,
 					[
 						'user' => $userId,
 						// parent share
@@ -307,10 +313,8 @@ class MountProvider implements IMountProvider, IPartialMountProvider {
 					],
 					$loader,
 					$view,
-					$foldersExistCache,
 					$this->eventDispatcher,
 					$user,
-					$shareId <= $maxValidatedShare,
 				);
 
 				$newMaxValidatedShare = max($shareId, $newMaxValidatedShare);

apps/files_sharing/lib/ShareTargetValidator.php

@@ -0,0 +1,163 @@
+<?php
+
+declare(strict_types=1);
+/**
+ * SPDX-FileCopyrightText: 2025 Nextcloud GmbH and Nextcloud contributors
+ * SPDX-License-Identifier: AGPL-3.0-or-later
+ */
+
+namespace OCA\Files_Sharing;
+
+use OC\Files\Filesystem;
+use OC\Files\SetupManager;
+use OC\Files\View;
+use OCP\Cache\CappedMemoryCache;
+use OCP\EventDispatcher\IEventDispatcher;
+use OCP\Files\Mount\IMountManager;
+use OCP\Files\Mount\IMountPoint;
+use OCP\IUser;
+use OCP\Share\Events\VerifyMountPointEvent;
+use OCP\Share\IManager;
+use OCP\Share\IShare;
+
+/**
+ * Validate that mount target is valid
+ */
+class ShareTargetValidator {
+	private CappedMemoryCache $folderExistsCache;
+
+	public function __construct(
+		private readonly IManager $shareManager,
+		private readonly IEventDispatcher $eventDispatcher,
+		private readonly SetupManager $setupManager,
+		private readonly IMountManager $mountManager,
+	) {
+		$this->folderExistsCache = new CappedMemoryCache();
+	}
+
+	private function getViewForUser(IUser $user): View {
+		/**
+		 * @psalm-suppress InternalClass
+		 * @psalm-suppress InternalMethod
+		 */
+		return new View('/' . $user->getUID() . '/files');
+	}
+
+	/**
+	 * check if the parent folder exists otherwise move the mount point up
+	 *
+	 * @param array<string, IMountPoint> $allCachedMounts Other mounts for the user, indexed by path
+	 * @param IShare[] $childShares
+	 * @return string
+	 */
+	public function verifyMountPoint(
+		IUser $user,
+		IShare &$share,
+		array $allCachedMounts,
+		array $childShares,
+	): string {
+		$mountPoint = basename($share->getTarget());
+		$parent = dirname($share->getTarget());
+
+		$recipientView = $this->getViewForUser($user);
+		$event = new VerifyMountPointEvent($share, $recipientView, $parent);
+		$this->eventDispatcher->dispatchTyped($event);
+		$parent = $event->getParent();
+
+		/** @psalm-suppress InternalMethod */
+		$absoluteParent = $recipientView->getAbsolutePath($parent);
+		$this->setupManager->setupForPath($absoluteParent);
+		$parentMount = $this->mountManager->find($absoluteParent);
+
+		$cached = $this->folderExistsCache->get($parent);
+		if ($cached !== null) {
+			$parentExists = $cached;
+		} else {
+			$parentCache = $parentMount->getStorage()->getCache();
+			$parentExists = $parentCache->inCache($parentMount->getInternalPath($absoluteParent));
+			$this->folderExistsCache->set($parent, $parentExists);
+		}
+		if (!$parentExists) {
+			$parent = Helper::getShareFolder($recipientView, $user->getUID());
+			/** @psalm-suppress InternalMethod */
+			$absoluteParent = $recipientView->getAbsolutePath($parent);
+		}
+
+		$newAbsoluteMountPoint = $this->generateUniqueTarget(
+			$share,
+			Filesystem::normalizePath($absoluteParent . '/' . $mountPoint),
+			$parentMount,
+			$allCachedMounts,
+		);
+
+		/** @psalm-suppress InternalMethod */
+		$newMountPoint = $recipientView->getRelativePath($newAbsoluteMountPoint);
+		if ($newMountPoint === null) {
+			return $share->getTarget();
+		}
+
+		if ($newMountPoint !== $share->getTarget()) {
+			$this->updateFileTarget($user, $newMountPoint, $share, $childShares);
+		}
+
+		return $newMountPoint;
+	}
+
+
+	/**
+	 * @param IMountPoint[] $allCachedMounts
+	 */
+	private function generateUniqueTarget(
+		IShare $share,
+		string $absolutePath,
+		IMountPoint $parentMount,
+		array $allCachedMounts,
+	): string {
+		$pathInfo = pathinfo($absolutePath);
+		$ext = isset($pathInfo['extension']) ? '.' . $pathInfo['extension'] : '';
+		$name = $pathInfo['filename'];
+		$dir = $pathInfo['dirname'];
+
+		$i = 2;
+		$parentCache = $parentMount->getStorage()->getCache();
+		$internalPath = $parentMount->getInternalPath($absolutePath);
+		while ($parentCache->inCache($internalPath) || $this->hasConflictingMount($share, $allCachedMounts, $absolutePath)) {
+			$absolutePath = Filesystem::normalizePath($dir . '/' . $name . ' (' . $i . ')' . $ext);
+			$internalPath = $parentMount->getInternalPath($absolutePath);
+			$i++;
+		}
+
+		return $absolutePath;
+	}
+
+	/**
+	 * @param IMountPoint[] $allCachedMounts
+	 */
+	private function hasConflictingMount(IShare $share, array $allCachedMounts, string $absolutePath): bool {
+		if (!isset($allCachedMounts[$absolutePath . '/'])) {
+			return false;
+		}
+
+		$mount = $allCachedMounts[$absolutePath . '/'];
+		if ($mount instanceof SharedMount && $mount->getShare()->getNodeId() === $share->getNodeId()) {
+			// "conflicting" mount is a mount for the current share
+			return false;
+		}
+
+		return true;
+	}
+
+	/**
+	 * update fileTarget in the database if the mount point changed
+	 *
+	 * @param IShare[] $childShares
+	 */
+	private function updateFileTarget(IUser $user, string $newPath, IShare &$share, array $childShares) {
+		$share->setTarget($newPath);
+
+		foreach ($childShares as $tmpShare) {
+			$tmpShare->setTarget($newPath);
+			$this->shareManager->moveShare($tmpShare, $user->getUID());
+		}
+	}
+}

apps/files_sharing/lib/SharedMount.php

@@ -13,14 +13,12 @@ use OC\Files\Mount\MountPoint;
 use OC\Files\Mount\MoveableMount;
 use OC\Files\View;
 use OCA\Files_Sharing\Exceptions\BrokenPath;
-use OCP\Cache\CappedMemoryCache;
 use OCP\EventDispatcher\IEventDispatcher;
 use OCP\Files\Events\InvalidateMountCacheEvent;
 use OCP\Files\Storage\IStorageFactory;
 use OCP\IDBConnection;
 use OCP\IUser;
 use OCP\Server;
-use OCP\Share\Events\VerifyMountPointEvent;
 use OCP\Share\IShare;
 use Psr\Log\LoggerInterface;
 
@@ -41,73 +39,20 @@ class SharedMount extends MountPoint implements MoveableMount, ISharedMountPoint
 
 	public function __construct(
 		$storage,
-		array $mountpoints,
 		$arguments,
 		IStorageFactory $loader,
 		private View $recipientView,
-		CappedMemoryCache $folderExistCache,
 		private IEventDispatcher $eventDispatcher,
 		private IUser $user,
-		bool $alreadyVerified,
 	) {
 		$this->superShare = $arguments['superShare'];
 		$this->groupedShares = $arguments['groupedShares'];
 
 		$absMountPoint = '/' . $user->getUID() . '/files/' . trim($this->superShare->getTarget(), '/') . '/';
 
-		// after the mountpoint is verified for the first time, only new mountpoints (e.g. groupfolders can overwrite the target)
-		if (!$alreadyVerified || isset($mountpoints[$absMountPoint])) {
-			$newMountPoint = $this->verifyMountPoint($this->superShare, $mountpoints, $folderExistCache);
-			$absMountPoint = '/' . $user->getUID() . '/files/' . trim($newMountPoint, '/') . '/';
-		}
-
 		parent::__construct($storage, $absMountPoint, $arguments, $loader, null, null, MountProvider::class);
 	}
 
-	/**
-	 * check if the parent folder exists otherwise move the mount point up
-	 *
-	 * @param IShare $share
-	 * @param SharedMount[] $mountpoints
-	 * @param CappedMemoryCache<bool> $folderExistCache
-	 * @return string
-	 */
-	private function verifyMountPoint(
-		IShare $share,
-		array $mountpoints,
-		CappedMemoryCache $folderExistCache,
-	) {
-		$mountPoint = basename($share->getTarget());
-		$parent = dirname($share->getTarget());
-
-		$event = new VerifyMountPointEvent($share, $this->recipientView, $parent);
-		$this->eventDispatcher->dispatchTyped($event);
-		$parent = $event->getParent();
-
-		$cached = $folderExistCache->get($parent);
-		if ($cached) {
-			$parentExists = $cached;
-		} else {
-			$parentExists = $this->recipientView->is_dir($parent);
-			$folderExistCache->set($parent, $parentExists);
-		}
-		if (!$parentExists) {
-			$parent = Helper::getShareFolder($this->recipientView, $this->user->getUID());
-		}
-
-		$newMountPoint = $this->generateUniqueTarget(
-			Filesystem::normalizePath($parent . '/' . $mountPoint),
-			$this->recipientView,
-			$mountpoints
-		);
-
-		if ($newMountPoint !== $share->getTarget()) {
-			$this->updateFileTarget($newMountPoint, $share);
-		}
-
-		return $newMountPoint;
-	}
-
 	/**
 	 * update fileTarget in the database if the mount point changed
 	 *
@@ -126,30 +71,6 @@ class SharedMount extends MountPoint implements MoveableMount, ISharedMountPoint
 		$this->eventDispatcher->dispatchTyped(new InvalidateMountCacheEvent($this->user));
 	}
 
-
-	/**
-	 * @param string $path
-	 * @param View $view
-	 * @param SharedMount[] $mountpoints
-	 * @return mixed
-	 */
-	private function generateUniqueTarget($path, $view, array $mountpoints) {
-		$pathinfo = pathinfo($path);
-		$ext = isset($pathinfo['extension']) ? '.' . $pathinfo['extension'] : '';
-		$name = $pathinfo['filename'];
-		$dir = $pathinfo['dirname'];
-
-		$i = 2;
-		$absolutePath = $this->recipientView->getAbsolutePath($path) . '/';
-		while ($view->file_exists($path) || isset($mountpoints[$absolutePath])) {
-			$path = Filesystem::normalizePath($dir . '/' . $name . ' (' . $i . ')' . $ext);
-			$absolutePath = $this->recipientView->getAbsolutePath($path) . '/';
-			$i++;
-		}
-
-		return $path;
-	}
-
 	/**
 	 * Format a path to be relative to the /user/files/ directory
 	 *

apps/files_sharing/tests/MountProviderTest.php

@@ -10,6 +10,7 @@ namespace OCA\Files_Sharing\Tests;
 use OC\Share20\Share;
 use OCA\Files_Sharing\MountProvider;
 use OCA\Files_Sharing\SharedMount;
+use OCA\Files_Sharing\ShareTargetValidator;
 use OCP\EventDispatcher\IEventDispatcher;
 use OCP\Files\Cache\ICacheEntry;
 use OCP\Files\Config\ICachedMountInfo;
@@ -54,8 +55,9 @@ class MountProviderTest extends \Test\TestCase {
 		$cacheFactory = $this->createMock(ICacheFactory::class);
 		$cacheFactory->method('createLocal')->willReturn($this->cache);
 		$mountManager = $this->createMock(IMountManager::class);
+		$shareValidator = $this->createMock(ShareTargetValidator::class);
 
-		$this->provider = new MountProvider($this->config, $this->shareManager, $this->logger, $eventDispatcher, $cacheFactory, $mountManager);
+		$this->provider = new MountProvider($this->config, $this->shareManager, $this->logger, $eventDispatcher, $cacheFactory, $mountManager, $shareValidator);
 	}
 
 	private function makeMockShareAttributes($attrs) {

build/psalm-baseline.xml

@@ -1663,12 +1663,6 @@
     </MoreSpecificImplementedParamType>
   </file>
   <file src="apps/files_sharing/lib/SharedMount.php">
-    <InternalMethod>
-      <code><![CDATA[file_exists]]></code>
-      <code><![CDATA[getAbsolutePath]]></code>
-      <code><![CDATA[getAbsolutePath]]></code>
-      <code><![CDATA[is_dir]]></code>
-    </InternalMethod>
     <InvalidReturnType>
       <code><![CDATA[bool]]></code>
     </InvalidReturnType>