Skip to content

Commit fb6d331

Browse files
TobyTheHuttTobyTheHutt
committed
feat(dav): serialize concurrent MOVE/COPY
Assisted-by: ClaudeCode:claude-opus-4-7 Signed-off-by: Tobias Harnickell <tobias.harnickell@bedag.ch>
1 parent 7924c2f commit fb6d331

7 files changed

Lines changed: 286 additions & 0 deletions

File tree

‎apps/dav/composer/composer/autoload_classmap.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -251,6 +251,7 @@
251251
'OCA\\DAV\\Connector\\Sabre\\PublicAuth' => $baseDir . '/../lib/Connector/Sabre/PublicAuth.php',
252252
'OCA\\DAV\\Connector\\Sabre\\QuotaPlugin' => $baseDir . '/../lib/Connector/Sabre/QuotaPlugin.php',
253253
'OCA\\DAV\\Connector\\Sabre\\RequestIdHeaderPlugin' => $baseDir . '/../lib/Connector/Sabre/RequestIdHeaderPlugin.php',
254+
'OCA\\DAV\\Connector\\Sabre\\SerializeMoveCopyPlugin' => $baseDir . '/../lib/Connector/Sabre/SerializeMoveCopyPlugin.php',
254255
'OCA\\DAV\\Connector\\Sabre\\Server' => $baseDir . '/../lib/Connector/Sabre/Server.php',
255256
'OCA\\DAV\\Connector\\Sabre\\ServerFactory' => $baseDir . '/../lib/Connector/Sabre/ServerFactory.php',
256257
'OCA\\DAV\\Connector\\Sabre\\ShareTypeList' => $baseDir . '/../lib/Connector/Sabre/ShareTypeList.php',

‎apps/dav/composer/composer/autoload_static.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -266,6 +266,7 @@ class ComposerStaticInitDAV
266266
'OCA\\DAV\\Connector\\Sabre\\PublicAuth' => __DIR__ . '/..' . '/../lib/Connector/Sabre/PublicAuth.php',
267267
'OCA\\DAV\\Connector\\Sabre\\QuotaPlugin' => __DIR__ . '/..' . '/../lib/Connector/Sabre/QuotaPlugin.php',
268268
'OCA\\DAV\\Connector\\Sabre\\RequestIdHeaderPlugin' => __DIR__ . '/..' . '/../lib/Connector/Sabre/RequestIdHeaderPlugin.php',
269+
'OCA\\DAV\\Connector\\Sabre\\SerializeMoveCopyPlugin' => __DIR__ . '/..' . '/../lib/Connector/Sabre/SerializeMoveCopyPlugin.php',
269270
'OCA\\DAV\\Connector\\Sabre\\Server' => __DIR__ . '/..' . '/../lib/Connector/Sabre/Server.php',
270271
'OCA\\DAV\\Connector\\Sabre\\ServerFactory' => __DIR__ . '/..' . '/../lib/Connector/Sabre/ServerFactory.php',
271272
'OCA\\DAV\\Connector\\Sabre\\ShareTypeList' => __DIR__ . '/..' . '/../lib/Connector/Sabre/ShareTypeList.php',

‎apps/dav/lib/Connector/Sabre/SerializeMoveCopyPlugin.php‎

Lines changed: 105 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,105 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
/**
5+
* SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors
6+
* SPDX-License-Identifier: AGPL-3.0-only
7+
*/
8+
9+
namespace OCA\DAV\Connector\Sabre;
10+
11+
use OCA\DAV\Connector\Sabre\Exception\FileLocked;
12+
use OCP\IConfig;
13+
use OCP\Lock\ILockingProvider;
14+
use OCP\Lock\LockedException;
15+
use Sabre\DAV\Server;
16+
use Sabre\DAV\ServerPlugin;
17+
use Sabre\HTTP\RequestInterface;
18+
use Sabre\HTTP\ResponseInterface;
19+
20+
/**
21+
* Serialize concurrent WebDAV MOVE and COPY on the same source or destination path.
22+
* Enable via 'dav.serialize_move_copy'.
23+
*/
24+
class SerializeMoveCopyPlugin extends ServerPlugin {
25+
/** Distinct namespace from the storage-layer View locks on the same path. */
26+
private const LOCK_KEY_PREFIX = 'webdav-serialize:';
27+
28+
private const CONFIG_KEY = 'dav.serialize_move_copy';
29+
30+
/** @var list<array{key: string, type: int}> */
31+
private array $heldLocks = [];
32+
33+
public function __construct(
34+
private ILockingProvider $lockingProvider,
35+
private IConfig $config,
36+
) {
37+
}
38+
39+
#[\Override]
40+
public function initialize(Server $server): void {
41+
$server->on('beforeMove', [$this, 'beforeMove']);
42+
$server->on('beforeCopy', [$this, 'beforeCopy']);
43+
$server->on('afterMethod:MOVE', [$this, 'afterMethod']);
44+
$server->on('afterMethod:COPY', [$this, 'afterMethod']);
45+
$server->on('exception', [$this, 'onException']);
46+
}
47+
48+
/** @throws FileLocked when the source or destination is contended. */
49+
public function beforeMove(string $source, string $destination): bool {
50+
return $this->guard($source, $destination, ILockingProvider::LOCK_EXCLUSIVE);
51+
}
52+
53+
/** @throws FileLocked when the source or destination is contended. */
54+
public function beforeCopy(string $source, string $destination): bool {
55+
return $this->guard($source, $destination, ILockingProvider::LOCK_SHARED);
56+
}
57+
58+
private function guard(string $source, string $destination, int $sourceLockType): bool {
59+
if (!$this->config->getSystemValueBool(self::CONFIG_KEY, false)) {
60+
return true;
61+
}
62+
$srcKey = self::LOCK_KEY_PREFIX . $source;
63+
$dstKey = self::LOCK_KEY_PREFIX . $destination;
64+
if ($srcKey === $dstKey) {
65+
return true;
66+
}
67+
// Path sort ensures two concurrent operations with swapped source and destination acquire in the same order.
68+
$order = strcmp($srcKey, $dstKey) < 0
69+
? [[$srcKey, $sourceLockType, $source], [$dstKey, ILockingProvider::LOCK_EXCLUSIVE, $destination]]
70+
: [[$dstKey, ILockingProvider::LOCK_EXCLUSIVE, $destination], [$srcKey, $sourceLockType, $source]];
71+
try {
72+
foreach ($order as [$key, $type, $readablePath]) {
73+
$this->lockingProvider->acquireLock($key, $type, $readablePath);
74+
$this->heldLocks[] = ['key' => $key, 'type' => $type];
75+
}
76+
} catch (LockedException $e) {
77+
$this->release();
78+
throw new FileLocked($e->getMessage(), $e->getCode(), $e);
79+
}
80+
return true;
81+
}
82+
83+
public function afterMethod(RequestInterface $request, ResponseInterface $response): void {
84+
$this->release();
85+
}
86+
87+
public function onException(\Throwable $exception): void {
88+
// afterMethod does not fire on exception. Release here so locks do not leak.
89+
$this->release();
90+
}
91+
92+
public function __destruct() {
93+
$this->release();
94+
}
95+
96+
private function release(): void {
97+
while ($lock = array_pop($this->heldLocks)) {
98+
try {
99+
$this->lockingProvider->releaseLock($lock['key'], $lock['type']);
100+
} catch (\Throwable) {
101+
// Multiple release() calls are expected across the hooks and destructor. Suppress to stay idempotent.
102+
}
103+
}
104+
}
105+
}

‎apps/dav/lib/Connector/Sabre/ServerFactory.php‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -107,6 +107,10 @@ public function createServer(
107107
$server->addPlugin(new DummyGetResponsePlugin());
108108
$server->addPlugin(new ExceptionLoggerPlugin('webdav', $this->logger));
109109
$server->addPlugin(new LockPlugin());
110+
$server->addPlugin(new SerializeMoveCopyPlugin(
111+
\OCP\Server::get(\OCP\Lock\ILockingProvider::class),
112+
$this->config,
113+
));
110114

111115
$server->addPlugin(new RequestIdHeaderPlugin($this->request));
112116
$server->addPlugin(new UserIdHeaderPlugin($this->userSession));

‎apps/dav/lib/Server.php‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -191,6 +191,10 @@ public function __construct(
191191

192192
$this->server->addPlugin(new ExceptionLoggerPlugin('webdav', $logger));
193193
$this->server->addPlugin(new LockPlugin());
194+
$this->server->addPlugin(new \OCA\DAV\Connector\Sabre\SerializeMoveCopyPlugin(
195+
\OCP\Server::get(\OCP\Lock\ILockingProvider::class),
196+
\OCP\Server::get(\OCP\IConfig::class),
197+
));
194198
$this->server->addPlugin(new \Sabre\DAV\Sync\Plugin());
195199

196200
// acl

‎apps/dav/tests/unit/Connector/Sabre/SerializeMoveCopyPluginTest.php‎

Lines changed: 161 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,161 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
/**
5+
* SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors
6+
* SPDX-License-Identifier: AGPL-3.0-only
7+
*/
8+
9+
namespace OCA\DAV\Tests\unit\Connector\Sabre;
10+
11+
use OCA\DAV\Connector\Sabre\Exception\FileLocked;
12+
use OCA\DAV\Connector\Sabre\SerializeMoveCopyPlugin;
13+
use OCP\IConfig;
14+
use OCP\Lock\ILockingProvider;
15+
use OCP\Lock\LockedException;
16+
use PHPUnit\Framework\MockObject\MockObject;
17+
use Sabre\DAV\Server;
18+
use Sabre\HTTP\Request;
19+
use Sabre\HTTP\Response;
20+
use Test\TestCase;
21+
22+
class SerializeMoveCopyPluginTest extends TestCase {
23+
private const LOCK_KEY_PREFIX = 'webdav-serialize:';
24+
private const CONFIG_KEY = 'dav.serialize_move_copy';
25+
26+
private ILockingProvider&MockObject $lockingProvider;
27+
private IConfig&MockObject $config;
28+
private SerializeMoveCopyPlugin $plugin;
29+
30+
protected function setUp(): void {
31+
parent::setUp();
32+
$this->lockingProvider = $this->createMock(ILockingProvider::class);
33+
$this->config = $this->createMock(IConfig::class);
34+
$this->plugin = new SerializeMoveCopyPlugin($this->lockingProvider, $this->config);
35+
}
36+
37+
private function toggle(bool $enabled): void {
38+
$this->config->method('getSystemValueBool')->with(self::CONFIG_KEY, false)->willReturn($enabled);
39+
}
40+
41+
/** @param list<array{key: string, type: int}> $calls appended by the mock callback in call order */
42+
private function captureAcquireCalls(array &$calls): void {
43+
$this->lockingProvider
44+
->method('acquireLock')
45+
->willReturnCallback(function (string $key, int $type) use (&$calls): void {
46+
$calls[] = ['key' => $key, 'type' => $type];
47+
});
48+
}
49+
50+
public function testInitializeSubscribesToExpectedEvents(): void {
51+
$server = new Server();
52+
$this->plugin->initialize($server);
53+
foreach (['beforeMove', 'beforeCopy', 'afterMethod:MOVE', 'afterMethod:COPY', 'exception'] as $event) {
54+
$this->assertNotEmpty($server->listeners($event), "no listener registered for $event");
55+
}
56+
}
57+
58+
public function testToggleOffIsNoop(): void {
59+
$this->toggle(false);
60+
$this->lockingProvider->expects($this->never())->method('acquireLock');
61+
$this->assertTrue($this->plugin->beforeMove('files/a/src.txt', 'files/a/dst.txt'));
62+
$this->assertTrue($this->plugin->beforeCopy('files/a/src.txt', 'files/a/dst.txt'));
63+
}
64+
65+
public function testMoveAcquiresExclusiveOnBothInSortedOrder(): void {
66+
$this->toggle(true);
67+
$calls = [];
68+
$this->captureAcquireCalls($calls);
69+
$this->plugin->beforeMove('files/a/1-src.txt', 'files/a/2-dst.txt');
70+
$this->assertSame([
71+
['key' => self::LOCK_KEY_PREFIX . 'files/a/1-src.txt', 'type' => ILockingProvider::LOCK_EXCLUSIVE],
72+
['key' => self::LOCK_KEY_PREFIX . 'files/a/2-dst.txt', 'type' => ILockingProvider::LOCK_EXCLUSIVE],
73+
], $calls);
74+
}
75+
76+
public function testCopyAcquiresSharedSourceAndExclusiveDestination(): void {
77+
$this->toggle(true);
78+
$calls = [];
79+
$this->captureAcquireCalls($calls);
80+
$this->plugin->beforeCopy('files/a/1-src.txt', 'files/a/2-dst.txt');
81+
$this->assertSame([
82+
['key' => self::LOCK_KEY_PREFIX . 'files/a/1-src.txt', 'type' => ILockingProvider::LOCK_SHARED],
83+
['key' => self::LOCK_KEY_PREFIX . 'files/a/2-dst.txt', 'type' => ILockingProvider::LOCK_EXCLUSIVE],
84+
], $calls);
85+
}
86+
87+
public function testAcquisitionOrderFollowsPathSortNotArgumentOrder(): void {
88+
// destination sorts before source alphabetically. The plugin MUST still lock destination first.
89+
$this->toggle(true);
90+
$calls = [];
91+
$this->captureAcquireCalls($calls);
92+
$this->plugin->beforeMove('files/a/z-src.txt', 'files/a/a-dst.txt');
93+
$this->assertSame([
94+
self::LOCK_KEY_PREFIX . 'files/a/a-dst.txt',
95+
self::LOCK_KEY_PREFIX . 'files/a/z-src.txt',
96+
], array_column($calls, 'key'));
97+
}
98+
99+
public function testSourceEqualsDestinationShortCircuits(): void {
100+
$this->toggle(true);
101+
$this->lockingProvider->expects($this->never())->method('acquireLock');
102+
$this->assertTrue($this->plugin->beforeMove('files/a/src.txt', 'files/a/src.txt'));
103+
$this->assertTrue($this->plugin->beforeCopy('files/a/src.txt', 'files/a/src.txt'));
104+
}
105+
106+
public function testLockedExceptionOnFirstLockMapsTo423(): void {
107+
$this->toggle(true);
108+
$this->lockingProvider->method('acquireLock')
109+
->willThrowException(new LockedException('files/a/src.txt'));
110+
$this->lockingProvider->expects($this->never())->method('releaseLock');
111+
112+
$this->expectException(FileLocked::class);
113+
try {
114+
$this->plugin->beforeMove('files/a/src.txt', 'files/a/dst.txt');
115+
} catch (FileLocked $e) {
116+
$this->assertSame(423, $e->getHTTPCode());
117+
throw $e;
118+
}
119+
}
120+
121+
public function testLockedExceptionOnSecondLockRollsBackFirst(): void {
122+
$this->toggle(true);
123+
$callCount = 0;
124+
$this->lockingProvider->expects($this->exactly(2))
125+
->method('acquireLock')
126+
->willReturnCallback(function () use (&$callCount): void {
127+
$callCount++;
128+
if ($callCount === 2) {
129+
throw new LockedException('files/a/2-dst.txt');
130+
}
131+
});
132+
$this->lockingProvider->expects($this->once())
133+
->method('releaseLock')
134+
->with(self::LOCK_KEY_PREFIX . 'files/a/1-src.txt', ILockingProvider::LOCK_EXCLUSIVE);
135+
136+
$this->expectException(FileLocked::class);
137+
$this->plugin->beforeMove('files/a/1-src.txt', 'files/a/2-dst.txt');
138+
}
139+
140+
public function testAfterMethodReleasesAllHeldLocks(): void {
141+
$this->toggle(true);
142+
$this->lockingProvider->expects($this->exactly(2))->method('releaseLock');
143+
$this->plugin->beforeMove('files/a/1-src.txt', 'files/a/2-dst.txt');
144+
$this->plugin->afterMethod(new Request('MOVE', 'files/a/1-src.txt'), new Response());
145+
}
146+
147+
public function testOnExceptionReleasesAllHeldLocks(): void {
148+
$this->toggle(true);
149+
$this->lockingProvider->expects($this->exactly(2))->method('releaseLock');
150+
$this->plugin->beforeCopy('files/a/1-src.txt', 'files/a/2-dst.txt');
151+
$this->plugin->onException(new \RuntimeException('boom'));
152+
}
153+
154+
public function testReleaseIsIdempotent(): void {
155+
$this->toggle(true);
156+
$this->lockingProvider->expects($this->exactly(2))->method('releaseLock');
157+
$this->plugin->beforeMove('files/a/1-src.txt', 'files/a/2-dst.txt');
158+
$this->plugin->afterMethod(new Request('MOVE', 'files/a/1-src.txt'), new Response());
159+
$this->plugin->onException(new \RuntimeException('later'));
160+
}
161+
}

‎config/config.sample.php‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2740,6 +2740,16 @@
27402740
*/
27412741
'filelocking.debug' => false,
27422742

2743+
/**
2744+
* Serialize concurrent WebDAV MOVE and COPY on the same source or destination
2745+
* path. When ``true``, the server MUST reject a conflicting concurrent
2746+
* operation with HTTP 423 Locked. Concurrent COPY from one source to
2747+
* different destinations remains allowed.
2748+
*
2749+
* Defaults to ``false``
2750+
*/
2751+
'dav.serialize_move_copy' => false,
2752+
27432753
/**
27442754
* Disable the web-based updater.
27452755
*

0 commit comments

Comments
 (0)