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..46706c12 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) ?? ''), + '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/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())); + } +} 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/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); 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');