From 5ebfdd4130352196724cebeee03d16103505687e Mon Sep 17 00:00:00 2001 From: Git'Fellow <12234510+solracsf@users.noreply.github.com> Date: Sun, 6 Sep 2026 17:28:46 +0200 Subject: [PATCH 1/3] fix(ocs): answer a lock conflict with the lock instead of a TypeError Since the CoreQueryBuilder removal, FileLock::import() expects database column names, but the controller feeds it the object itself when a lock is refused and the jsonSerialize() shape when an unlock is refused. Both 423 answers therefore died with a TypeError and the client saw a 500 with no lock in it. The same mismatch broke the remote lock read for federated shares, which passes its own array. Keep the lock as an object until the responder serializes it, and let import() accept the shape jsonSerialize() produces as well as the database one. Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com> --- lib/Controller/LockController.php | 27 ++-- lib/Model/FileLock.php | 21 +-- lib/Service/LockService.php | 7 +- tests/Feature/ControllableTimeFactory.php | 39 ++++++ tests/Feature/LockTestCase.php | 160 ++++++++++++++++++++++ tests/Feature/OcsControllerTest.php | 84 ++++++++++++ tests/bootstrap.php | 2 + 7 files changed, 316 insertions(+), 24 deletions(-) create mode 100644 tests/Feature/ControllableTimeFactory.php create mode 100644 tests/Feature/LockTestCase.php create mode 100644 tests/Feature/OcsControllerTest.php diff --git a/lib/Controller/LockController.php b/lib/Controller/LockController.php index 9a4b1b86..c07d159e 100644 --- a/lib/Controller/LockController.php +++ b/lib/Controller/LockController.php @@ -103,11 +103,14 @@ public function unlocking(string $fileId, int $lockType = ILock::TYPE_USER): Dat $response->setStatus(Http::STATUS_PRECONDITION_FAILED); return $response; } catch (UnauthorizedUnlockException) { - $lock = $this->lockService->getLockFromFileId((int)$fileId); - $response = new DataResponse(); - $response->setStatus(Http::STATUS_LOCKED); - $response->setData($lock->jsonSerialize()); - return $response; + try { + $lock = $this->lockService->getLockFromFileId((int)$fileId); + } catch (LockNotFoundException) { + $response = new DataResponse(); + $response->setStatus(Http::STATUS_PRECONDITION_FAILED); + return $response; + } + return new DataResponse($lock, Http::STATUS_LOCKED); } catch (Exception $e) { return $this->fail($e); } @@ -120,21 +123,17 @@ public function setOCSVersion($version): void { private function buildOCSResponse(string $format, DataResponse $data): V1Response|V2Response { $message = null; - if ($data->getStatus() === Http::STATUS_LOCKED) { - $lock = new FileLock(); - $lock->import($data->getData()); - $this->lockService->injectMetadata($lock); - $message = $this->l10n->t('File is currently locked by %s', [$lock->getDisplayName()]); + $containedData = $data->getData(); + if ($data->getStatus() === Http::STATUS_LOCKED && $containedData instanceof FileLock) { + $this->lockService->injectMetadata($containedData); + $message = $this->l10n->t('File is currently locked by %s', [$containedData->getDisplayName() ?? $containedData->getOwner()]); } if ($data->getStatus() === Http::STATUS_PRECONDITION_FAILED) { - /** @var FileLock $lock */ - $lock = $data->getData(); $message = $this->l10n->t('File is not locked'); } - $containedData = $data->getData(); if ($containedData instanceof FileLock) { - $data->setData($data->getData()->jsonSerialize()); + $data->setData($containedData->jsonSerialize()); } if ($this->ocsVersion === 1) { diff --git a/lib/Model/FileLock.php b/lib/Model/FileLock.php index 6a180215..98612f97 100644 --- a/lib/Model/FileLock.php +++ b/lib/Model/FileLock.php @@ -203,16 +203,19 @@ public function importFromDatabase(array $data): self { return $this; } + /** + * Import the shape produced by jsonSerialize() (also accepts database column names). + */ public function import(array $data): void { - $this->setId((int)$data['id']); - $this->setUri($data['uri'] ?? ''); - $this->setUserId($data['user_id']); - $this->setFileId((int)$data['file_id']); - $this->setToken($data['token'] ?? ''); - $this->setCreation((int)$data['creation']); - $this->setLockType((int)$data['type']); - $this->setTimeout((int)$data['ttl']); - $this->setDisplayName($data['owner'] ?? ''); + $this->setId((int)($data['id'] ?? 0)); + $this->setUri((string)($data['uri'] ?? '')); + $this->setUserId((string)($data['userId'] ?? $data['user_id'] ?? '')); + $this->setFileId((int)($data['fileId'] ?? $data['file_id'] ?? 0)); + $this->setToken((string)($data['token'] ?? '')); + $this->setCreation((int)($data['creation'] ?? 0)); + $this->setLockType((int)($data['type'] ?? ILock::TYPE_USER)); + $this->setTimeout((int)($data['ttl'] ?? 0)); + $this->setDisplayName((string)($data['displayName'] ?? $data['owner'] ?? '')); } #[\Override] diff --git a/lib/Service/LockService.php b/lib/Service/LockService.php index 80ac5393..7f4519b8 100644 --- a/lib/Service/LockService.php +++ b/lib/Service/LockService.php @@ -56,6 +56,11 @@ public function __construct( ) { } + public function clearCache(): void { + $this->lockCache = []; + $this->remoteLockCache = []; + } + public function getLockForNodeId(int $nodeId, ?Node $node = null): FileLock|false { if (array_key_exists($nodeId, $this->lockCache) && $this->lockCache[$nodeId] !== false) { return $this->lockCache[$nodeId]; @@ -415,7 +420,7 @@ public function getRemoteLockFromDav(int $nodeId, ?Node $node = null): ?FileLock $fileLock = new FileLock(); $fileLock->import([ 'fileId' => $nodeId, - 'owner' => (string)($storage->getPropfindPropertyValue($path, Application::DAV_PROPERTY_LOCK_OWNER_DISPLAYNAME) ?? ''), + 'userId' => (string)($storage->getPropfindPropertyValue($path, Application::DAV_PROPERTY_LOCK_OWNER_DISPLAYNAME) ?? ''), 'type' => (int)($storage->getPropfindPropertyValue($path, Application::DAV_PROPERTY_LOCK_OWNER_TYPE) ?? 0), 'creation' => (int)($storage->getPropfindPropertyValue($path, Application::DAV_PROPERTY_LOCK_TIME) ?? 0), 'ttl' => (int)($storage->getPropfindPropertyValue($path, Application::DAV_PROPERTY_LOCK_TIMEOUT) ?? 0), diff --git a/tests/Feature/ControllableTimeFactory.php b/tests/Feature/ControllableTimeFactory.php new file mode 100644 index 00000000..8a40ecb7 --- /dev/null +++ b/tests/Feature/ControllableTimeFactory.php @@ -0,0 +1,39 @@ +time ?? time(); + } + + #[\Override] + public function getDateTime(string $time = 'now', ?\DateTimeZone $timezone = null): \DateTime { + if ($time === 'now' && $this->time !== null) { + return (new \DateTime('@' . $this->time))->setTimezone($timezone ?? new \DateTimeZone('UTC')); + } + return parent::getDateTime($time, $timezone); + } + + #[\Override] + public function now(): \DateTimeImmutable { + return new \DateTimeImmutable('@' . $this->getTime()); + } +} diff --git a/tests/Feature/LockTestCase.php b/tests/Feature/LockTestCase.php new file mode 100644 index 00000000..e12ab681 --- /dev/null +++ b/tests/Feature/LockTestCase.php @@ -0,0 +1,160 @@ +createUser($user, $user); + } + \OCP\Server::get(IUserManager::class)->registerBackend($backend); + } + + protected function setUp(): void { + parent::setUp(); + $this->time = null; + $this->lockManager = \OCP\Server::get(ILockManager::class); + $this->rootFolder = \OCP\Server::get(IRootFolder::class); + $this->timeFactory = new ControllableTimeFactory(); + $this->overwriteService(ITimeFactory::class, $this->timeFactory); + $this->clearLocks(); + $this->setLockTimeoutMinutes(-1); + \OC_Hook::$thrownExceptions = []; + } + + protected function tearDown(): void { + $this->clearLocks(); + foreach ([self::USER1, self::USER2, self::USER3] as $user) { + try { + $this->loginAsUser($user); + foreach ($this->rootFolder->getUserFolder($user)->getDirectoryListing() as $node) { + try { + $node->delete(); + } catch (\Throwable) { + } + } + if (class_exists(\OCA\Files_Trashbin\Trashbin::class)) { + \OCA\Files_Trashbin\Trashbin::deleteAll(); + } + } catch (\Throwable) { + } + } + parent::tearDown(); + } + + protected function lockService(): LockService { + return \OCP\Server::get(LockService::class); + } + + protected function clearLocks(): void { + \OCP\Server::get(IDBConnection::class)->executeStatement('DELETE FROM `*PREFIX*files_lock`'); + $this->lockService()->clearCache(); + } + + protected function setLockTimeoutMinutes(int $minutes): void { + \OCP\Server::get(IConfig::class)->setAppValue(Application::APP_ID, ConfigLexicon::LOCK_TIMEOUT, (string)$minutes); + } + + protected function loginAndGetUserFolder(string $userId): Folder { + $this->loginAsUser($userId); + $this->lockService()->clearCache(); + return $this->rootFolder->getUserFolder($userId); + } + + protected function shareWith(\OCP\Files\Node $node, string $owner, string $user, int $permissions = 19): IShare { + $shareManager = \OCP\Server::get(IShareManager::class); + $share = $shareManager->newShare(); + $share->setNode($node) + ->setSharedBy($owner) + ->setSharedWith($user) + ->setShareType(IShare::TYPE_USER) + ->setPermissions($permissions); + $share = $shareManager->createShare($share); + $share->setStatus(IShare::STATUS_ACCEPTED); + $shareManager->updateShare($share); + return $share; + } + + /** + * Move the clock to an absolute moment, which lets a test model two processes + * whose clock reads happen in a different order than their database writes. + */ + protected function atTime(int $timestamp): void { + $this->time = $timestamp; + $this->timeFactory->time = $timestamp; + $this->lockService()->clearCache(); + } + + protected function toTheFuture(int $seconds): void { + if ($this->time === null) { + $this->time = time(); + } + $this->time += $seconds; + $this->timeFactory->time = $this->time; + $this->lockService()->clearCache(); + } + + protected function lockRowCount(int $fileId): int { + return (int)\OCP\Server::get(IDBConnection::class)->executeQuery( + 'SELECT COUNT(*) FROM `*PREFIX*files_lock` WHERE `file_id` = ?', [$fileId] + )->fetchOne(); + } + + protected function storedLock(int $fileId): ?FileLock { + $locks = \OCP\Server::get(LocksRequest::class)->getFromFileIds([$fileId]); + return $locks[0] ?? null; + } + + /** + * Create a file as USER1 and share it with USER2 (and optionally USER3). + */ + protected function sharedFile(string $name, int $permissions = 19, ?int $permissionsUser3 = null): File { + $file = $this->loginAndGetUserFolder(self::USER1)->newFile($name, 'AAA'); + $this->shareWith($file, self::USER1, self::USER2, $permissions); + if ($permissionsUser3 !== null) { + $this->shareWith($file, self::USER1, self::USER3, $permissionsUser3); + } + return $file; + } +} diff --git a/tests/Feature/OcsControllerTest.php b/tests/Feature/OcsControllerTest.php new file mode 100644 index 00000000..b112ecfe --- /dev/null +++ b/tests/Feature/OcsControllerTest.php @@ -0,0 +1,84 @@ +setOCSVersion(2); + return $controller; + } + + /** + * @return array{int, array|string} rendered status and decoded body (array for json, string for xml) + */ + private function render(DataResponse $response, string $format = 'json'): array { + $rendered = $this->controller()->buildResponse($response, $format); + self::assertInstanceOf(BaseResponse::class, $rendered); + $body = $rendered->render(); + if ($format === 'json') { + return [$rendered->getStatus(), json_decode($body, true, 512, JSON_THROW_ON_ERROR)]; + } + return [$rendered->getStatus(), $body]; + } + + public function testLockUnlockRoundTrip(): void { + $file = $this->loginAndGetUserFolder(self::USER1)->newFile('ocs.txt', 'AAA'); + [$status, $body] = $this->render($this->controller()->locking((string)$file->getId())); + self::assertSame(Http::STATUS_OK, $status); + self::assertSame(self::USER1, $body['ocs']['data']['userId']); + self::assertSame(ILock::TYPE_USER, $body['ocs']['data']['type']); + self::assertStringStartsWith('files_lock/', $body['ocs']['data']['token']); + + [$status] = $this->render($this->controller()->unlocking((string)$file->getId())); + self::assertSame(Http::STATUS_OK, $status); + self::assertSame(0, $this->lockRowCount($file->getId())); + + [$status, $body] = $this->render($this->controller()->unlocking((string)$file->getId())); + self::assertSame(Http::STATUS_PRECONDITION_FAILED, $status); + self::assertSame('File is not locked', $body['ocs']['meta']['message']); + } + + public function testConflictIsAStructured423(): void { + $file = $this->sharedFile('conflict.txt'); + $this->lockManager->lock(new LockContext($file, ILock::TYPE_USER, self::USER1)); + $this->loginAndGetUserFolder(self::USER2); + + foreach (['json', 'xml'] as $format) { + [$status, $body] = $this->render($this->controller()->locking((string)$file->getId()), $format); + self::assertSame(Http::STATUS_LOCKED, $status, $format); + if ($format === 'json') { + self::assertSame(self::USER1, $body['ocs']['data']['userId']); + self::assertSame($file->getId(), $body['ocs']['data']['fileId']); + self::assertStringContainsString('locked by', $body['ocs']['meta']['message']); + } else { + self::assertStringContainsString('' . self::USER1 . '', $body); + self::assertStringContainsString('423', $body); + } + } + + [$status, $body] = $this->render($this->controller()->unlocking((string)$file->getId())); + self::assertSame(Http::STATUS_LOCKED, $status, 'a recipient may not release the owner lock'); + self::assertSame(self::USER1, $body['ocs']['data']['userId']); + self::assertSame(1, $this->lockRowCount($file->getId())); + } +} diff --git a/tests/bootstrap.php b/tests/bootstrap.php index f1414fbe..062a5fd0 100644 --- a/tests/bootstrap.php +++ b/tests/bootstrap.php @@ -17,4 +17,6 @@ require_once __DIR__ . '/../../../lib/base.php'; require_once __DIR__ . '/../../../tests/autoload.php'; +\OC::$composerAutoloader->addPsr4('OCA\\FilesLock\\Tests\\', __DIR__ . '/', true); + Server::get(IAppManager::class)->loadApp('files_lock'); From 120a96cb95e0376ddbdf94c7c2c410cb4daba501 Mon Sep 17 00:00:00 2001 From: Git'Fellow <12234510+solracsf@users.noreply.github.com> Date: Mon, 7 Sep 2026 17:51:32 +0200 Subject: [PATCH 2/3] fix(dav): keep the remote display name on a federated lock Spotted by @benjaminfrueh in review. Making import() tolerant moved this value to the wrong field. The name a federated lock carries comes from the remote as free text, so it belongs in displayName, which is where the old 'owner' key put it. As 'userId' it left displayName empty, and the line below appends the host to it, so the web UI showed a lock owned by plain "@remotehost". It is also not a local user id. getOwner() feeds nc:lock-owner, which the frontend compares against the current user to decide whether to offer unlock, so a remote name that happened to match a local uid would have offered it wrongly. Nothing sets userId for a remote lock now, as before. Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com> --- lib/Service/LockService.php | 2 +- tests/Feature/LockFeatureTest.php | 38 +++++++++++++++++++++++++++++++ 2 files changed, 39 insertions(+), 1 deletion(-) diff --git a/lib/Service/LockService.php b/lib/Service/LockService.php index 7f4519b8..46706c12 100644 --- a/lib/Service/LockService.php +++ b/lib/Service/LockService.php @@ -420,7 +420,7 @@ public function getRemoteLockFromDav(int $nodeId, ?Node $node = null): ?FileLock $fileLock = new FileLock(); $fileLock->import([ 'fileId' => $nodeId, - 'userId' => (string)($storage->getPropfindPropertyValue($path, Application::DAV_PROPERTY_LOCK_OWNER_DISPLAYNAME) ?? ''), + 'displayName' => (string)($storage->getPropfindPropertyValue($path, Application::DAV_PROPERTY_LOCK_OWNER_DISPLAYNAME) ?? ''), 'type' => (int)($storage->getPropfindPropertyValue($path, Application::DAV_PROPERTY_LOCK_OWNER_TYPE) ?? 0), 'creation' => (int)($storage->getPropfindPropertyValue($path, Application::DAV_PROPERTY_LOCK_TIME) ?? 0), 'ttl' => (int)($storage->getPropfindPropertyValue($path, Application::DAV_PROPERTY_LOCK_TIMEOUT) ?? 0), diff --git a/tests/Feature/LockFeatureTest.php b/tests/Feature/LockFeatureTest.php index 9fced4c4..0fb0954f 100644 --- a/tests/Feature/LockFeatureTest.php +++ b/tests/Feature/LockFeatureTest.php @@ -456,6 +456,44 @@ public function testUnlockStaleClientLock(): void { $this->assertCount(0, $locks); } + /** + * The display name of a federated lock comes from the remote as free text. + * It is not a local user id and must not be treated as one. + */ + public function testRemoteLockKeepsTheRemoteDisplayName(): void { + $this->loginAsUser(self::TEST_USER1); + + $storage = $this->createMock(\OCA\Files_Sharing\External\Storage::class); + $storage->method('instanceOfStorage')->willReturnCallback( + static fn (string $class): bool => $class === \OC\Files\Storage\DAV::class + ); + $storage->method('getPropfindPropertyValue')->willReturnCallback( + static fn (string $path, string $property): mixed => match ($property) { + Application::DAV_PROPERTY_LOCK => '1', + Application::DAV_PROPERTY_LOCK_OWNER_DISPLAYNAME => 'Alice Remote', + Application::DAV_PROPERTY_LOCK_OWNER_TYPE => (string)ILock::TYPE_USER, + default => null, + } + ); + $storage->method('getRemote')->willReturn('https://cloud.example.org/remote.php/dav'); + + $node = $this->createMock(\OCP\Files\Node::class); + $node->method('getStorage')->willReturn($storage); + $node->method('getInternalPath')->willReturn('files/remote-locked.txt'); + + $service = \OCP\Server::get(LockService::class); + $lock = $service->getRemoteLockFromDav(424242, $node); + + $this->assertNotNull($lock); + $this->assertSame('Alice Remote@cloud.example.org', $lock->getDisplayName()); + $this->assertSame('', $lock->getOwner(), 'a remote display name is not a local user id'); + $this->assertSame( + 'Alice Remote@cloud.example.org', + $service->injectMetadata($lock)->getDisplayName(), + 'the propfind path runs every lock through injectMetadata' + ); + } + private function loginAndGetUserFolder(string $userId) { $this->loginAsUser($userId); return $this->rootFolder->getUserFolder($userId); From de66797d9adc70bf3acbad3ce430c0841e278890 Mon Sep 17 00:00:00 2001 From: Git'Fellow <12234510+solracsf@users.noreply.github.com> Date: Mon, 7 Sep 2026 20:35:20 +0200 Subject: [PATCH 3/3] test: cover the files:lock command The command had no test at all. This drives it through CommandAdapter, the same wrapper the console registers it with, so the attribute-based signature is exercised as the runtime sees it. Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com> --- tests/Feature/CommandTest.php | 53 +++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) create mode 100644 tests/Feature/CommandTest.php diff --git a/tests/Feature/CommandTest.php b/tests/Feature/CommandTest.php new file mode 100644 index 00000000..696e030c --- /dev/null +++ b/tests/Feature/CommandTest.php @@ -0,0 +1,53 @@ +loginAndGetUserFolder(self::USER1)->newFile('cli.txt', 'AAA'); + $id = $file->getId(); + $tester = $this->tester(); + + $tester->execute(['file_id' => (string)$id, '--status' => true]); + self::assertStringContainsString('not locked', $tester->getDisplay()); + + self::assertSame(0, $tester->execute(['file_id' => (string)$id, 'user_id' => self::USER1])); + self::assertSame(1, $this->lockRowCount($id)); + + $tester->execute(['file_id' => (string)$id, '--status' => true]); + self::assertStringContainsString('locked by ' . self::USER1, $tester->getDisplay()); + } + + public function testUnlock(): void { + $file = $this->loginAndGetUserFolder(self::USER1)->newFile('cli-unlock.txt', 'AAA'); + $this->lockManager->lock(new LockContext($file, ILock::TYPE_USER, self::USER1)); + + self::assertSame(0, $this->tester()->execute(['file_id' => (string)$file->getId(), '--unlock' => true])); + self::assertSame(0, $this->lockRowCount($file->getId())); + } +}