From e6cf2c47f7a05453171cd023ede5c0f161abf876 Mon Sep 17 00:00:00 2001 From: Josh Date: Fri, 24 Jul 2026 09:14:21 -0400 Subject: [PATCH 1/3] fix(files_sharing): roll back shared storage lock on owner lock failure Assisted-by: Copilot:GPT-5 Signed-off-by: Josh --- apps/files_sharing/lib/SharedStorage.php | 41 +++++++++++++++++++++--- 1 file changed, 36 insertions(+), 5 deletions(-) diff --git a/apps/files_sharing/lib/SharedStorage.php b/apps/files_sharing/lib/SharedStorage.php index 51f94c54782cb..0444257d50f1c 100644 --- a/apps/files_sharing/lib/SharedStorage.php +++ b/apps/files_sharing/lib/SharedStorage.php @@ -493,10 +493,31 @@ public function acquireLock(string $path, int $type, ILockingProvider $provider) /** @var ILockingStorage $targetStorage */ [$targetStorage, $targetInternalPath] = $this->resolvePath($path); $targetStorage->acquireLock($targetInternalPath, $type, $provider); - // lock the parent folders of the owner when locking the share as recipient + + // Lock the owner parent folders when locking the share root as recipient. if ($path === '') { $sourcePath = $this->ownerUserFolder->getRelativePath($this->sourcePath); - $this->ownerView->lockFile(dirname($sourcePath), ILockingProvider::LOCK_SHARED, true); + $ownerParentPath = dirname($sourcePath); + + try { + $this->ownerView->lockFile($ownerParentPath, ILockingProvider::LOCK_SHARED, true); + } catch (\Throwable $e) { + // Do not leave the source-node lock behind if locking its parent fails. + try { + $targetStorage->releaseLock($targetInternalPath, $type, $provider); + } catch (\Throwable $releaseException) { + $this->logger->error( + 'Failed to roll back share lock after locking the owner parent failed', + [ + 'app' => 'files_sharing', + 'exception' => $releaseException, + 'shareId' => $this->getShareId(), + ] + ); + } + + throw $e; + } } } @@ -504,12 +525,22 @@ public function acquireLock(string $path, int $type, ILockingProvider $provider) public function releaseLock(string $path, int $type, ILockingProvider $provider): void { /** @var ILockingStorage $targetStorage */ [$targetStorage, $targetInternalPath] = $this->resolvePath($path); - $targetStorage->releaseLock($targetInternalPath, $type, $provider); - // unlock the parent folders of the owner when unlocking the share as recipient + if ($path === '') { $sourcePath = $this->ownerUserFolder->getRelativePath($this->sourcePath); - $this->ownerView->unlockFile(dirname($sourcePath), ILockingProvider::LOCK_SHARED, true); + $ownerParentPath = dirname($sourcePath); + + // Release in reverse acquisition order. Always release the target lock, + // even if unlocking the owner parent fails. + try { + $this->ownerView->unlockFile($ownerParentPath, ILockingProvider::LOCK_SHARED, true); + } finally { + $targetStorage->releaseLock($targetInternalPath, $type, $provider); + } + return; } + + $targetStorage->releaseLock($targetInternalPath, $type, $provider); } #[\Override] From c4cdce84c861519f2cc345d4596eb6ab48f905fa Mon Sep 17 00:00:00 2001 From: Josh Date: Fri, 24 Jul 2026 09:18:27 -0400 Subject: [PATCH 2/3] test(files_sharing): cover shared storage lock rollback Assisted-by: Copilot:GPT-5.6 Signed-off-by: Josh --- .../files_sharing/tests/SharedStorageTest.php | 132 ++++++++++++++++++ 1 file changed, 132 insertions(+) diff --git a/apps/files_sharing/tests/SharedStorageTest.php b/apps/files_sharing/tests/SharedStorageTest.php index 26e1e0a5e81c0..8451f869d373b 100644 --- a/apps/files_sharing/tests/SharedStorageTest.php +++ b/apps/files_sharing/tests/SharedStorageTest.php @@ -18,8 +18,13 @@ use OCP\AppFramework\Bootstrap\IBootContext; use OCP\Constants; use OCP\Files\Config\IMountProviderCollection; +use OCP\Files\Folder; +use OCP\Files\IRootFolder; +use OCP\Files\Node; use OCP\Files\NotFoundException; +use OCP\Files\Storage\IStorage; use OCP\IUserManager; +use OCP\Lock\ILockingProvider; use OCP\Server; use OCP\Share\IShare; @@ -502,4 +507,131 @@ public function testCopyPermissions(): void { $this->view->unlink($this->filename); $this->shareManager->deleteShare($share); } + + public function testAcquireRootLockRollsBackTargetLockWhenOwnerParentLockFails(): void { + $events = []; + $provider = $this->createMock(ILockingProvider::class); + $ownerView = $this->createMock(View::class); + $sourceStorage = $this->createMock(IStorage::class); + + $sourceStorage->expects($this->once()) + ->method('acquireLock') + ->with('foo/bar.txt', ILockingProvider::LOCK_SHARED, $provider) + ->willReturnCallback(function () use (&$events): void { + $events[] = 'target-acquire'; + }); + $sourceStorage->expects($this->once()) + ->method('releaseLock') + ->with('foo/bar.txt', ILockingProvider::LOCK_SHARED, $provider) + ->willReturnCallback(function () use (&$events): void { + $events[] = 'target-release'; + }); + + $ownerView->expects($this->once()) + ->method('lockFile') + ->with('foo', ILockingProvider::LOCK_SHARED, true) + ->willReturnCallback(function () use (&$events): void { + $events[] = 'owner-lock'; + throw new \RuntimeException('Owner parent is locked'); + }); + + $storage = $this->createSharedStorageForRootLocking($ownerView, $sourceStorage); + + try { + $storage->acquireLock('', ILockingProvider::LOCK_SHARED, $provider); + $this->fail('Expected owner-parent locking to fail'); + } catch (\RuntimeException $e) { + $this->assertSame('Owner parent is locked', $e->getMessage()); + } + + $this->assertSame([ + 'target-acquire', + 'owner-lock', + 'target-release', + ], $events); + } + + public function testReleaseRootLockUnlocksOwnerParentBeforeTarget(): void { + $events = []; + $provider = $this->createMock(ILockingProvider::class); + $ownerView = $this->createMock(View::class); + $sourceStorage = $this->createMock(IStorage::class); + + $ownerView->expects($this->once()) + ->method('unlockFile') + ->with('foo', ILockingProvider::LOCK_SHARED, true) + ->willReturnCallback(function () use (&$events): void { + $events[] = 'owner-release'; + }); + $sourceStorage->expects($this->once()) + ->method('releaseLock') + ->with('foo/bar.txt', ILockingProvider::LOCK_SHARED, $provider) + ->willReturnCallback(function () use (&$events): void { + $events[] = 'target-release'; + }); + + $storage = $this->createSharedStorageForRootLocking($ownerView, $sourceStorage); + $storage->releaseLock('', ILockingProvider::LOCK_SHARED, $provider); + + $this->assertSame([ + 'owner-release', + 'target-release', + ], $events); + } + + public function testReleaseRootLockStillReleasesTargetWhenOwnerParentUnlockFails(): void { + $provider = $this->createMock(ILockingProvider::class); + $ownerView = $this->createMock(View::class); + $sourceStorage = $this->createMock(IStorage::class); + + $ownerView->expects($this->once()) + ->method('unlockFile') + ->with('foo', ILockingProvider::LOCK_SHARED, true) + ->willThrowException(new \RuntimeException('Owner parent unlock failed')); + $sourceStorage->expects($this->once()) + ->method('releaseLock') + ->with('foo/bar.txt', ILockingProvider::LOCK_SHARED, $provider); + + $storage = $this->createSharedStorageForRootLocking($ownerView, $sourceStorage); + + $this->expectException(\RuntimeException::class); + $this->expectExceptionMessage('Owner parent unlock failed'); + + $storage->releaseLock('', ILockingProvider::LOCK_SHARED, $provider); + } + + private function createSharedStorageForRootLocking( + View $ownerView, + IStorage $sourceStorage, + ): SharedStorage { + $share = $this->createMock(IShare::class); + $share->method('getShareOwner')->willReturn(self::TEST_FILES_SHARING_API_USER1); + $share->method('getNodeId')->willReturn(42); + $share->method('getPermissions')->willReturn(Constants::PERMISSION_ALL); + + $sourceNode = $this->createMock(Node::class); + $sourceNode->method('getStorage')->willReturn($sourceStorage); + $sourceNode->method('getPath') + ->willReturn('/' . self::TEST_FILES_SHARING_API_USER1 . '/files/foo/bar.txt'); + $sourceNode->method('getInternalPath')->willReturn('foo/bar.txt'); + + $ownerUserFolder = $this->createMock(Folder::class); + $ownerUserFolder->method('getById')->with(42)->willReturn([$sourceNode]); + $ownerUserFolder->method('getRelativePath')->with( + '/' . self::TEST_FILES_SHARING_API_USER1 . '/files/foo/bar.txt' + )->willReturn('foo/bar.txt'); + + $rootFolder = $this->createMock(IRootFolder::class); + $rootFolder->method('getUserFolder') + ->with(self::TEST_FILES_SHARING_API_USER1) + ->willReturn($ownerUserFolder); + + return new SharedStorage([ + 'ownerView' => $ownerView, + 'rootFolder' => $rootFolder, + 'superShare' => $share, + 'groupedShares' => [$share], + 'user' => self::TEST_FILES_SHARING_API_USER2, + ]); + } } From 2873a44b23f4030d06594fb91741697d354213d3 Mon Sep 17 00:00:00 2001 From: Josh Date: Fri, 24 Jul 2026 09:56:21 -0400 Subject: [PATCH 3/3] test(files_sharing): cover missing shared storage source Assisted-by: Copilot:GPT-5.6 Signed-off-by: Josh --- apps/files_sharing/tests/SharedStorageTest.php | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/apps/files_sharing/tests/SharedStorageTest.php b/apps/files_sharing/tests/SharedStorageTest.php index 8451f869d373b..eb9f64b7d2429 100644 --- a/apps/files_sharing/tests/SharedStorageTest.php +++ b/apps/files_sharing/tests/SharedStorageTest.php @@ -467,11 +467,17 @@ public function testInitWithNonExistingUser(): void { public function testInitWithNotFoundSource(): void { $share = $this->createMock(IShare::class); $share->method('getShareOwner')->willReturn(self::TEST_FILES_SHARING_API_USER1); - $share->method('getNodeId')->willReturn(1); - $ownerView = $this->createMock(View::class); - $ownerView->method('getPath')->willThrowException(new NotFoundException()); + $share->method('getNodeId')->willReturn(42); + + $rootFolder = $this->createMock(IRootFolder::class); + $rootFolder->expects($this->once()) + ->method('getUserFolder') + ->with(self::TEST_FILES_SHARING_API_USER1) + ->willReturn($ownerUserFolder); + $storage = new SharedStorage([ - 'ownerView' => $ownerView, + 'ownerView' => $this->createMock(View::class), + 'rootFolder' => $rootFolder, 'superShare' => $share, 'groupedShares' => [$share], 'user' => 'user1', @@ -480,6 +486,9 @@ public function testInitWithNotFoundSource(): void { // trigger init $this->assertInstanceOf(FailedStorage::class, $storage->getSourceStorage()); $this->assertInstanceOf(FailedCache::class, $storage->getCache()); + + $this->assertSame(0, $storage->getPermissions()); + $this->assertFalse($storage->isReadable('missing.txt')); } public function testCopyPermissions(): void {