From 78e37361c735663be15d2b360a350b8266f09cab Mon Sep 17 00:00:00 2001 From: Tobias Harnickell Date: Thu, 30 Jul 2026 15:15:45 +0200 Subject: [PATCH] feat(dav): serialize concurrent MOVE/COPY Assisted-by: ClaudeCode:claude-opus-4-7 Signed-off-by: Tobias Harnickell --- .../composer/composer/autoload_classmap.php | 1 + .../dav/composer/composer/autoload_static.php | 1 + .../Sabre/SerializeMoveCopyPlugin.php | 105 ++++++++++++ .../dav/lib/Connector/Sabre/ServerFactory.php | 4 + apps/dav/lib/Server.php | 4 + .../Sabre/SerializeMoveCopyPluginTest.php | 161 ++++++++++++++++++ config/config.sample.php | 10 ++ 7 files changed, 286 insertions(+) create mode 100644 apps/dav/lib/Connector/Sabre/SerializeMoveCopyPlugin.php create mode 100644 apps/dav/tests/unit/Connector/Sabre/SerializeMoveCopyPluginTest.php diff --git a/apps/dav/composer/composer/autoload_classmap.php b/apps/dav/composer/composer/autoload_classmap.php index ef25c658b83a4..22d0971e6da16 100644 --- a/apps/dav/composer/composer/autoload_classmap.php +++ b/apps/dav/composer/composer/autoload_classmap.php @@ -251,6 +251,7 @@ 'OCA\\DAV\\Connector\\Sabre\\PublicAuth' => $baseDir . '/../lib/Connector/Sabre/PublicAuth.php', 'OCA\\DAV\\Connector\\Sabre\\QuotaPlugin' => $baseDir . '/../lib/Connector/Sabre/QuotaPlugin.php', 'OCA\\DAV\\Connector\\Sabre\\RequestIdHeaderPlugin' => $baseDir . '/../lib/Connector/Sabre/RequestIdHeaderPlugin.php', + 'OCA\\DAV\\Connector\\Sabre\\SerializeMoveCopyPlugin' => $baseDir . '/../lib/Connector/Sabre/SerializeMoveCopyPlugin.php', 'OCA\\DAV\\Connector\\Sabre\\Server' => $baseDir . '/../lib/Connector/Sabre/Server.php', 'OCA\\DAV\\Connector\\Sabre\\ServerFactory' => $baseDir . '/../lib/Connector/Sabre/ServerFactory.php', 'OCA\\DAV\\Connector\\Sabre\\ShareTypeList' => $baseDir . '/../lib/Connector/Sabre/ShareTypeList.php', diff --git a/apps/dav/composer/composer/autoload_static.php b/apps/dav/composer/composer/autoload_static.php index 5aa979e007acc..0794c35ac670e 100644 --- a/apps/dav/composer/composer/autoload_static.php +++ b/apps/dav/composer/composer/autoload_static.php @@ -266,6 +266,7 @@ class ComposerStaticInitDAV 'OCA\\DAV\\Connector\\Sabre\\PublicAuth' => __DIR__ . '/..' . '/../lib/Connector/Sabre/PublicAuth.php', 'OCA\\DAV\\Connector\\Sabre\\QuotaPlugin' => __DIR__ . '/..' . '/../lib/Connector/Sabre/QuotaPlugin.php', 'OCA\\DAV\\Connector\\Sabre\\RequestIdHeaderPlugin' => __DIR__ . '/..' . '/../lib/Connector/Sabre/RequestIdHeaderPlugin.php', + 'OCA\\DAV\\Connector\\Sabre\\SerializeMoveCopyPlugin' => __DIR__ . '/..' . '/../lib/Connector/Sabre/SerializeMoveCopyPlugin.php', 'OCA\\DAV\\Connector\\Sabre\\Server' => __DIR__ . '/..' . '/../lib/Connector/Sabre/Server.php', 'OCA\\DAV\\Connector\\Sabre\\ServerFactory' => __DIR__ . '/..' . '/../lib/Connector/Sabre/ServerFactory.php', 'OCA\\DAV\\Connector\\Sabre\\ShareTypeList' => __DIR__ . '/..' . '/../lib/Connector/Sabre/ShareTypeList.php', diff --git a/apps/dav/lib/Connector/Sabre/SerializeMoveCopyPlugin.php b/apps/dav/lib/Connector/Sabre/SerializeMoveCopyPlugin.php new file mode 100644 index 0000000000000..2b3a89b0d4d0c --- /dev/null +++ b/apps/dav/lib/Connector/Sabre/SerializeMoveCopyPlugin.php @@ -0,0 +1,105 @@ + */ + private array $heldLocks = []; + + public function __construct( + private ILockingProvider $lockingProvider, + private IConfig $config, + ) { + } + + #[\Override] + public function initialize(Server $server): void { + $server->on('beforeMove', [$this, 'beforeMove']); + $server->on('beforeCopy', [$this, 'beforeCopy']); + $server->on('afterMethod:MOVE', [$this, 'afterMethod']); + $server->on('afterMethod:COPY', [$this, 'afterMethod']); + $server->on('exception', [$this, 'onException']); + } + + /** @throws FileLocked when the source or destination is contended. */ + public function beforeMove(string $source, string $destination): bool { + return $this->guard($source, $destination, ILockingProvider::LOCK_EXCLUSIVE); + } + + /** @throws FileLocked when the source or destination is contended. */ + public function beforeCopy(string $source, string $destination): bool { + return $this->guard($source, $destination, ILockingProvider::LOCK_SHARED); + } + + private function guard(string $source, string $destination, int $sourceLockType): bool { + if (!$this->config->getSystemValueBool(self::CONFIG_KEY, false)) { + return true; + } + $srcKey = self::LOCK_KEY_PREFIX . $source; + $dstKey = self::LOCK_KEY_PREFIX . $destination; + if ($srcKey === $dstKey) { + return true; + } + // Path sort ensures two concurrent operations with swapped source and destination acquire in the same order. + $order = strcmp($srcKey, $dstKey) < 0 + ? [[$srcKey, $sourceLockType, $source], [$dstKey, ILockingProvider::LOCK_EXCLUSIVE, $destination]] + : [[$dstKey, ILockingProvider::LOCK_EXCLUSIVE, $destination], [$srcKey, $sourceLockType, $source]]; + try { + foreach ($order as [$key, $type, $readablePath]) { + $this->lockingProvider->acquireLock($key, $type, $readablePath); + $this->heldLocks[] = ['key' => $key, 'type' => $type]; + } + } catch (LockedException $e) { + $this->release(); + throw new FileLocked($e->getMessage(), $e->getCode(), $e); + } + return true; + } + + public function afterMethod(RequestInterface $request, ResponseInterface $response): void { + $this->release(); + } + + public function onException(\Throwable $exception): void { + // afterMethod does not fire on exception. Release here so locks do not leak. + $this->release(); + } + + public function __destruct() { + $this->release(); + } + + private function release(): void { + while ($lock = array_pop($this->heldLocks)) { + try { + $this->lockingProvider->releaseLock($lock['key'], $lock['type']); + } catch (\Throwable) { + // Multiple release() calls are expected across the hooks and destructor. Suppress to stay idempotent. + } + } + } +} diff --git a/apps/dav/lib/Connector/Sabre/ServerFactory.php b/apps/dav/lib/Connector/Sabre/ServerFactory.php index cb0f1e7bfc6a9..3bd7f8c66606a 100644 --- a/apps/dav/lib/Connector/Sabre/ServerFactory.php +++ b/apps/dav/lib/Connector/Sabre/ServerFactory.php @@ -107,6 +107,10 @@ public function createServer( $server->addPlugin(new DummyGetResponsePlugin()); $server->addPlugin(new ExceptionLoggerPlugin('webdav', $this->logger)); $server->addPlugin(new LockPlugin()); + $server->addPlugin(new SerializeMoveCopyPlugin( + \OCP\Server::get(\OCP\Lock\ILockingProvider::class), + $this->config, + )); $server->addPlugin(new RequestIdHeaderPlugin($this->request)); $server->addPlugin(new UserIdHeaderPlugin($this->userSession)); diff --git a/apps/dav/lib/Server.php b/apps/dav/lib/Server.php index fdaa35e4a4d16..0c583e4ac316d 100644 --- a/apps/dav/lib/Server.php +++ b/apps/dav/lib/Server.php @@ -191,6 +191,10 @@ public function __construct( $this->server->addPlugin(new ExceptionLoggerPlugin('webdav', $logger)); $this->server->addPlugin(new LockPlugin()); + $this->server->addPlugin(new \OCA\DAV\Connector\Sabre\SerializeMoveCopyPlugin( + \OCP\Server::get(\OCP\Lock\ILockingProvider::class), + \OCP\Server::get(\OCP\IConfig::class), + )); $this->server->addPlugin(new \Sabre\DAV\Sync\Plugin()); // acl diff --git a/apps/dav/tests/unit/Connector/Sabre/SerializeMoveCopyPluginTest.php b/apps/dav/tests/unit/Connector/Sabre/SerializeMoveCopyPluginTest.php new file mode 100644 index 0000000000000..3727469733533 --- /dev/null +++ b/apps/dav/tests/unit/Connector/Sabre/SerializeMoveCopyPluginTest.php @@ -0,0 +1,161 @@ +lockingProvider = $this->createMock(ILockingProvider::class); + $this->config = $this->createMock(IConfig::class); + $this->plugin = new SerializeMoveCopyPlugin($this->lockingProvider, $this->config); + } + + private function toggle(bool $enabled): void { + $this->config->method('getSystemValueBool')->with(self::CONFIG_KEY, false)->willReturn($enabled); + } + + /** @param list $calls appended by the mock callback in call order */ + private function captureAcquireCalls(array &$calls): void { + $this->lockingProvider + ->method('acquireLock') + ->willReturnCallback(function (string $key, int $type) use (&$calls): void { + $calls[] = ['key' => $key, 'type' => $type]; + }); + } + + public function testInitializeSubscribesToExpectedEvents(): void { + $server = new Server(); + $this->plugin->initialize($server); + foreach (['beforeMove', 'beforeCopy', 'afterMethod:MOVE', 'afterMethod:COPY', 'exception'] as $event) { + $this->assertNotEmpty($server->listeners($event), "no listener registered for $event"); + } + } + + public function testToggleOffIsNoop(): void { + $this->toggle(false); + $this->lockingProvider->expects($this->never())->method('acquireLock'); + $this->assertTrue($this->plugin->beforeMove('files/a/src.txt', 'files/a/dst.txt')); + $this->assertTrue($this->plugin->beforeCopy('files/a/src.txt', 'files/a/dst.txt')); + } + + public function testMoveAcquiresExclusiveOnBothInSortedOrder(): void { + $this->toggle(true); + $calls = []; + $this->captureAcquireCalls($calls); + $this->plugin->beforeMove('files/a/1-src.txt', 'files/a/2-dst.txt'); + $this->assertSame([ + ['key' => self::LOCK_KEY_PREFIX . 'files/a/1-src.txt', 'type' => ILockingProvider::LOCK_EXCLUSIVE], + ['key' => self::LOCK_KEY_PREFIX . 'files/a/2-dst.txt', 'type' => ILockingProvider::LOCK_EXCLUSIVE], + ], $calls); + } + + public function testCopyAcquiresSharedSourceAndExclusiveDestination(): void { + $this->toggle(true); + $calls = []; + $this->captureAcquireCalls($calls); + $this->plugin->beforeCopy('files/a/1-src.txt', 'files/a/2-dst.txt'); + $this->assertSame([ + ['key' => self::LOCK_KEY_PREFIX . 'files/a/1-src.txt', 'type' => ILockingProvider::LOCK_SHARED], + ['key' => self::LOCK_KEY_PREFIX . 'files/a/2-dst.txt', 'type' => ILockingProvider::LOCK_EXCLUSIVE], + ], $calls); + } + + public function testAcquisitionOrderFollowsPathSortNotArgumentOrder(): void { + // destination sorts before source alphabetically. The plugin MUST still lock destination first. + $this->toggle(true); + $calls = []; + $this->captureAcquireCalls($calls); + $this->plugin->beforeMove('files/a/z-src.txt', 'files/a/a-dst.txt'); + $this->assertSame([ + self::LOCK_KEY_PREFIX . 'files/a/a-dst.txt', + self::LOCK_KEY_PREFIX . 'files/a/z-src.txt', + ], array_column($calls, 'key')); + } + + public function testSourceEqualsDestinationShortCircuits(): void { + $this->toggle(true); + $this->lockingProvider->expects($this->never())->method('acquireLock'); + $this->assertTrue($this->plugin->beforeMove('files/a/src.txt', 'files/a/src.txt')); + $this->assertTrue($this->plugin->beforeCopy('files/a/src.txt', 'files/a/src.txt')); + } + + public function testLockedExceptionOnFirstLockMapsTo423(): void { + $this->toggle(true); + $this->lockingProvider->method('acquireLock') + ->willThrowException(new LockedException('files/a/src.txt')); + $this->lockingProvider->expects($this->never())->method('releaseLock'); + + $this->expectException(FileLocked::class); + try { + $this->plugin->beforeMove('files/a/src.txt', 'files/a/dst.txt'); + } catch (FileLocked $e) { + $this->assertSame(423, $e->getHTTPCode()); + throw $e; + } + } + + public function testLockedExceptionOnSecondLockRollsBackFirst(): void { + $this->toggle(true); + $callCount = 0; + $this->lockingProvider->expects($this->exactly(2)) + ->method('acquireLock') + ->willReturnCallback(function () use (&$callCount): void { + $callCount++; + if ($callCount === 2) { + throw new LockedException('files/a/2-dst.txt'); + } + }); + $this->lockingProvider->expects($this->once()) + ->method('releaseLock') + ->with(self::LOCK_KEY_PREFIX . 'files/a/1-src.txt', ILockingProvider::LOCK_EXCLUSIVE); + + $this->expectException(FileLocked::class); + $this->plugin->beforeMove('files/a/1-src.txt', 'files/a/2-dst.txt'); + } + + public function testAfterMethodReleasesAllHeldLocks(): void { + $this->toggle(true); + $this->lockingProvider->expects($this->exactly(2))->method('releaseLock'); + $this->plugin->beforeMove('files/a/1-src.txt', 'files/a/2-dst.txt'); + $this->plugin->afterMethod(new Request('MOVE', 'files/a/1-src.txt'), new Response()); + } + + public function testOnExceptionReleasesAllHeldLocks(): void { + $this->toggle(true); + $this->lockingProvider->expects($this->exactly(2))->method('releaseLock'); + $this->plugin->beforeCopy('files/a/1-src.txt', 'files/a/2-dst.txt'); + $this->plugin->onException(new \RuntimeException('boom')); + } + + public function testReleaseIsIdempotent(): void { + $this->toggle(true); + $this->lockingProvider->expects($this->exactly(2))->method('releaseLock'); + $this->plugin->beforeMove('files/a/1-src.txt', 'files/a/2-dst.txt'); + $this->plugin->afterMethod(new Request('MOVE', 'files/a/1-src.txt'), new Response()); + $this->plugin->onException(new \RuntimeException('later')); + } +} diff --git a/config/config.sample.php b/config/config.sample.php index cadfa4821dc16..a142de1c30d58 100644 --- a/config/config.sample.php +++ b/config/config.sample.php @@ -2740,6 +2740,16 @@ */ 'filelocking.debug' => false, + /** + * Serialize concurrent WebDAV MOVE and COPY on the same source or destination + * path. When ``true``, the server MUST reject a conflicting concurrent + * operation with HTTP 423 Locked. Concurrent COPY from one source to + * different destinations remains allowed. + * + * Defaults to ``false`` + */ + 'dav.serialize_move_copy' => false, + /** * Disable the web-based updater. *