From d125e81ed132bb2ec2998730b28e7b89aeac6af5 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Tue, 8 Apr 2025 15:36:29 +0200 Subject: [PATCH 1/2] fix: more optimized caching for share target verification Signed-off-by: Robin Appelman --- apps/files_sharing/lib/MountProvider.php | 16 ++++++++++++-- apps/files_sharing/lib/SharedMount.php | 21 ++++++++----------- .../files_sharing/tests/MountProviderTest.php | 4 +++- 3 files changed, 26 insertions(+), 15 deletions(-) diff --git a/apps/files_sharing/lib/MountProvider.php b/apps/files_sharing/lib/MountProvider.php index ad9498371d33c..ea99023676bba 100644 --- a/apps/files_sharing/lib/MountProvider.php +++ b/apps/files_sharing/lib/MountProvider.php @@ -11,6 +11,7 @@ use OCP\Cache\CappedMemoryCache; use OCP\EventDispatcher\IEventDispatcher; use OCP\Files\Config\IMountProvider; +use OCP\Files\Mount\IMountManager; use OCP\Files\Mount\IMountPoint; use OCP\Files\Storage\IStorageFactory; use OCP\ICacheFactory; @@ -32,6 +33,7 @@ public function __construct( protected LoggerInterface $logger, protected IEventDispatcher $eventDispatcher, protected ICacheFactory $cacheFactory, + protected IMountManager $mountManager, ) { } @@ -58,12 +60,17 @@ public function getMountsForUser(IUser $user, IStorageFactory $loader) { $superShares = $this->buildSuperShares($shares, $user); - $mounts = []; + $mounts = $this->mountManager->getAll(); $view = new View('/' . $user->getUID() . '/files'); $ownerViews = []; $sharingDisabledForUser = $this->shareManager->sharingDisabledForUser($user->getUID()); /** @var CappedMemoryCache $folderExistCache */ $foldersExistCache = new CappedMemoryCache(); + + $validShareCache = $this->cacheFactory->createLocal('share-valid-mountpoint-max'); + $maxValidatedShare = $validShareCache->get($user->getUID()) ?? 0; + $newMaxValidatedShare = $maxValidatedShare; + foreach ($superShares as $share) { try { /** @var IShare $parentShare */ @@ -80,6 +87,7 @@ public function getMountsForUser(IUser $user, IStorageFactory $loader) { if (!isset($ownerViews[$owner])) { $ownerViews[$owner] = new View('/' . $parentShare->getShareOwner() . '/files'); } + $shareId = (int)$parentShare->getId(); $mount = new SharedMount( '\OCA\Files_Sharing\SharedStorage', $mounts, @@ -97,9 +105,11 @@ public function getMountsForUser(IUser $user, IStorageFactory $loader) { $foldersExistCache, $this->eventDispatcher, $user, - $this->cacheFactory->createLocal('share-valid-mountpoint') + ($shareId <= $maxValidatedShare) ); + $newMaxValidatedShare = max($shareId, $newMaxValidatedShare); + $event = new ShareMountedEvent($mount); $this->eventDispatcher->dispatchTyped($event); @@ -118,6 +128,8 @@ public function getMountsForUser(IUser $user, IStorageFactory $loader) { } } + $validShareCache->set($user->getUID(), $newMaxValidatedShare, 24 * 60 * 60); + // array_filter removes the null values from the array return array_values(array_filter($mounts)); } diff --git a/apps/files_sharing/lib/SharedMount.php b/apps/files_sharing/lib/SharedMount.php index 5f7e8f376f150..069efd0223096 100644 --- a/apps/files_sharing/lib/SharedMount.php +++ b/apps/files_sharing/lib/SharedMount.php @@ -16,7 +16,6 @@ use OCP\EventDispatcher\IEventDispatcher; use OCP\Files\Events\InvalidateMountCacheEvent; use OCP\Files\Storage\IStorageFactory; -use OCP\ICache; use OCP\IUser; use OCP\Server; use OCP\Share\Events\VerifyMountPointEvent; @@ -47,13 +46,19 @@ public function __construct( CappedMemoryCache $folderExistCache, private IEventDispatcher $eventDispatcher, private IUser $user, - private ICache $cache, + bool $alreadyVerified, ) { $this->superShare = $arguments['superShare']; $this->groupedShares = $arguments['groupedShares']; - $newMountPoint = $this->verifyMountPoint($this->superShare, $mountpoints, $folderExistCache); - $absMountPoint = '/' . $user->getUID() . '/files' . $newMountPoint; + $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); } @@ -70,12 +75,6 @@ private function verifyMountPoint( array $mountpoints, CappedMemoryCache $folderExistCache, ) { - $cacheKey = $this->user->getUID() . '/' . $share->getId() . '/' . $share->getTarget(); - $cached = $this->cache->get($cacheKey); - if ($cached !== null) { - return $cached; - } - $mountPoint = basename($share->getTarget()); $parent = dirname($share->getTarget()); @@ -104,8 +103,6 @@ private function verifyMountPoint( $this->updateFileTarget($newMountPoint, $share); } - $this->cache->set($cacheKey, $newMountPoint, 60 * 60); - return $newMountPoint; } diff --git a/apps/files_sharing/tests/MountProviderTest.php b/apps/files_sharing/tests/MountProviderTest.php index 1f67bdd01fab3..285af51f0220e 100644 --- a/apps/files_sharing/tests/MountProviderTest.php +++ b/apps/files_sharing/tests/MountProviderTest.php @@ -11,6 +11,7 @@ use OCA\Files_Sharing\MountProvider; use OCP\EventDispatcher\IEventDispatcher; use OCP\Files\IRootFolder; +use OCP\Files\Mount\IMountManager; use OCP\Files\Storage\IStorageFactory; use OCP\ICacheFactory; use OCP\IConfig; @@ -55,8 +56,9 @@ protected function setUp(): void { $cacheFactory = $this->createMock(ICacheFactory::class); $cacheFactory->method('createLocal') ->willReturn(new NullCache()); + $mountManager = $this->createMock(IMountManager::class); - $this->provider = new MountProvider($this->config, $this->shareManager, $this->logger, $eventDispatcher, $cacheFactory); + $this->provider = new MountProvider($this->config, $this->shareManager, $this->logger, $eventDispatcher, $cacheFactory, $mountManager); } private function makeMockShareAttributes($attrs) { From 51b52d824803a9f60ce22c839273b4a1d24eec0b Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Fri, 11 Apr 2025 14:20:10 +0200 Subject: [PATCH 2/2] fix: don't return other mounts from share mount provider Signed-off-by: Robin Appelman --- apps/files_sharing/lib/MountProvider.php | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/apps/files_sharing/lib/MountProvider.php b/apps/files_sharing/lib/MountProvider.php index ea99023676bba..91c392de6eb51 100644 --- a/apps/files_sharing/lib/MountProvider.php +++ b/apps/files_sharing/lib/MountProvider.php @@ -60,7 +60,8 @@ public function getMountsForUser(IUser $user, IStorageFactory $loader) { $superShares = $this->buildSuperShares($shares, $user); - $mounts = $this->mountManager->getAll(); + $otherMounts = $this->mountManager->getAll(); + $mounts = []; $view = new View('/' . $user->getUID() . '/files'); $ownerViews = []; $sharingDisabledForUser = $this->shareManager->sharingDisabledForUser($user->getUID()); @@ -90,7 +91,7 @@ public function getMountsForUser(IUser $user, IStorageFactory $loader) { $shareId = (int)$parentShare->getId(); $mount = new SharedMount( '\OCA\Files_Sharing\SharedStorage', - $mounts, + array_merge($mounts, $otherMounts), [ 'user' => $user->getUID(), // parent share @@ -105,7 +106,7 @@ public function getMountsForUser(IUser $user, IStorageFactory $loader) { $foldersExistCache, $this->eventDispatcher, $user, - ($shareId <= $maxValidatedShare) + ($shareId <= $maxValidatedShare), ); $newMaxValidatedShare = max($shareId, $newMaxValidatedShare);