diff --git a/lib/Service/LockService.php b/lib/Service/LockService.php index cbc643c6..15e7a082 100644 --- a/lib/Service/LockService.php +++ b/lib/Service/LockService.php @@ -120,7 +120,7 @@ public function getLockForNodeIds(array $nodeIds): array { $expiredLocks = []; foreach ($newLocks as $lock) { if ($lock->getETA() === 0) { - $expiredLocks[] = $lock->getId(); + $expiredLocks[] = $lock; $locks[$lock->getFileId()] = false; $this->lockCache[$lock->getFileId()] = false; } else { @@ -129,9 +129,7 @@ public function getLockForNodeIds(array $nodeIds): array { } } - if (count($expiredLocks) > 0) { - $this->locksRequest->removeIds($expiredLocks); - } + $this->removeLocks($expiredLocks); return $locks; } @@ -163,7 +161,7 @@ public function lock(LockContext $lockScope): FileLock { $this->logger->notice('locking file', ['fileLock' => $lock]); $this->injectMetadata($lock); $this->locksRequest->save($lock); - $this->propagateEtag($lockScope); + $this->propagateEtag($lockScope->getNode()); return $lock; } } @@ -194,7 +192,7 @@ public function unlock(LockContext $lock, bool $force = false): FileLock { $this->locksRequest->delete($known); $this->lockCache[$lock->getNode()->getId()] = false; - $this->propagateEtag($lock); + $this->propagateEtag($lock->getNode()); $this->injectMetadata($known); return $known; } @@ -278,7 +276,6 @@ public function unlockFile(int $fileId, ?string $userId, bool $force = false, in $lockType, $userId, ); - $this->propagateEtag($lock); return $this->unlock($lock, $force); } @@ -309,7 +306,7 @@ public function getDeprecatedLocks(int $limit = 0): array { public function getLockFromFileId(int $fileId): FileLock { $lock = $this->locksRequest->getFromFileId($fileId); if ($lock->getETA() === 0) { - $this->locksRequest->delete($lock); + $this->removeLocks([$lock]); throw new LockNotFoundException('lock is ignored and deleted as being too old.'); } @@ -377,6 +374,9 @@ public function removeLocks(array $locks): void { $this->logger->notice('removing locks', ['ids' => $ids]); $this->locksRequest->removeIds($ids); + foreach ($locks as $lock) { + $this->propagateEtagForLock($lock); + } } public function getRemoteLockFromDav(int $nodeId, ?Node $node = null): ?FileLock { @@ -436,8 +436,31 @@ public function getRemoteLockFromDav(int $nodeId, ?Node $node = null): ?FileLock } } - private function propagateEtag(LockContext $lockContext): void { - $node = $lockContext->getNode(); + /** + * A lock that goes away on its own has to move the etag the way an explicit + * unlock does, because sync clients only re-read the lock properties of a + * file whose etag changed. + * + * Runs without a session, for the cleanup job: the owner's folder gives a + * scoped lookup for a user lock, and the root falls back on the mount cache + * for an app owned lock, whose owner is an app id rather than an account. + */ + private function propagateEtagForLock(FileLock $lock): void { + try { + $node = null; + if ($this->userManager->userExists($lock->getOwner())) { + $node = $this->rootFolder->getUserFolder($lock->getOwner())->getFirstNodeById($lock->getFileId()); + } + $node ??= $this->rootFolder->getFirstNodeById($lock->getFileId()); + if ($node !== null) { + $this->propagateEtag($node); + } + } catch (Exception $e) { + $this->logger->warning('Failed to propagate the etag of an expired lock', ['exception' => $e]); + } + } + + private function propagateEtag(Node $node): void { $node->getStorage()->getCache()->update($node->getId(), [ 'etag' => uniqid(), ]); diff --git a/tests/Feature/LockFeatureTest.php b/tests/Feature/LockFeatureTest.php index 75a5047f..95681a23 100644 --- a/tests/Feature/LockFeatureTest.php +++ b/tests/Feature/LockFeatureTest.php @@ -11,6 +11,7 @@ use OCA\FilesLock\AppInfo\Application; use OCA\FilesLock\ConfigLexicon; use OCA\FilesLock\DAV\LockPlugin; +use OCA\FilesLock\Exceptions\LockNotFoundException; use OCA\FilesLock\Model\FileLock; use OCA\FilesLock\Service\LockService; use OCP\AppFramework\Utility\ITimeFactory; @@ -24,6 +25,7 @@ use OCP\Lock\ManuallyLockedException; use OCP\Share\IManager as IShareManager; use OCP\Share\IShare; +use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\Attributes\Group; use PHPUnit\Framework\MockObject\MockObject; use Sabre\DAV\Locks\LockInfo; @@ -189,6 +191,80 @@ public function testLockEtagShare(): void { self::assertNotEquals($oldEtag, $file->getEtag()); } + /** + * Every way a lock can be dropped for having expired. + * + * @return array> + */ + public static function expiryTriggers(): array { + return [ + 'the cleanup job' => [static function (LockService $service, int $fileId): void { + $service->removeLocks($service->getDeprecatedLocks()); + }], + 'a single read' => [static function (LockService $service, int $fileId): void { + try { + $service->getLockFromFileId($fileId); + } catch (LockNotFoundException) { + } + }], + 'the bulk read behind a directory PROPFIND' => [static function (LockService $service, int $fileId): void { + $service->getLockForNodeIds([$fileId]); + }], + ]; + } + + /** + * A lock that expires has to move the etag the way an explicit unlock does. + * Sync clients only re-read a file's lock properties when its etag changed, + * so without this the desktop keeps showing an expired lock and keeps the + * local file read-only until someone locks and unlocks it by hand. + * + * @param \Closure(LockService, int): void $expire + */ + #[DataProvider('expiryTriggers')] + public function testExpiredLockPropagatesEtag(\Closure $expire): void { + \OCP\Server::get(IConfig::class)->setAppValue(Application::APP_ID, ConfigLexicon::LOCK_TIMEOUT, 30); + $file = $this->loginAndGetUserFolder(self::TEST_USER1)->newFile('etag_test', 'etag_test'); + $this->lockManager->lock(new LockContext($file, ILock::TYPE_USER, self::TEST_USER1)); + + $file = $this->loginAndGetUserFolder(self::TEST_USER1)->get('etag_test'); + $fileId = $file->getId(); + $lockedEtag = $file->getEtag(); + $lockedRootEtag = $this->loginAndGetUserFolder(self::TEST_USER1)->getEtag(); + + $this->toTheFuture(30 * 60 + 1); + $service = \OCP\Server::get(LockService::class); + $service->clearCache(); + $expire($service, $fileId); + + $file = $this->loginAndGetUserFolder(self::TEST_USER1)->get('etag_test'); + $newRootEtag = $this->loginAndGetUserFolder(self::TEST_USER1)->getEtag(); + + self::assertNotEquals($lockedEtag, $file->getEtag(), 'the file etag must change when the lock expires'); + self::assertNotEquals($lockedRootEtag, $newRootEtag, 'and the change must propagate up the tree'); + } + + /** + * An app lock is owned by an app id, not by an account, so the file cannot be + * resolved through an owner's folder and has to come from the mount cache. + */ + public function testExpiredAppLockPropagatesEtag(): void { + \OCP\Server::get(IConfig::class)->setAppValue(Application::APP_ID, ConfigLexicon::LOCK_TIMEOUT, 30); + $file = $this->loginAndGetUserFolder(self::TEST_USER1)->newFile('etag_test', 'etag_test'); + $this->lockManager->lock(new LockContext($file, ILock::TYPE_APP, 'collaborative_app')); + + $file = $this->loginAndGetUserFolder(self::TEST_USER1)->get('etag_test'); + $lockedEtag = $file->getEtag(); + + $this->toTheFuture(30 * 60 + 1); + $service = \OCP\Server::get(LockService::class); + $service->clearCache(); + $service->removeLocks($service->getDeprecatedLocks()); + + $file = $this->loginAndGetUserFolder(self::TEST_USER1)->get('etag_test'); + self::assertNotEquals($lockedEtag, $file->getEtag(), 'an app owned lock has no account to resolve through'); + } + public function testUnlockEtagShare(): void { $file = $this->loginAndGetUserFolder(self::TEST_USER1) ->newFile('etag_test', 'etag_test');