Merge pull request #497 from nextcloud/fix/timeout-handling

fix(timeout): compare creation time to now - timeout
nextcloud · Feb 13, 2025 · 15123c9 · 15123c9
2 parents 84db68e + a26ce04
commit 15123c9
Show file tree

Hide file tree

Showing 5 changed files with 31 additions and 196 deletions.
diff --git a/lib/Controller/LockController.php b/lib/Controller/LockController.php
@@ -16,7 +16,6 @@
 use OCA\FilesLock\Model\FileLock;
 use OCA\FilesLock\Service\FileService;
 use OCA\FilesLock\Service\LockService;
-use OCA\FilesLock\Tools\Traits\TLogger;
 use OCP\AppFramework\Http;
 use OCP\AppFramework\Http\DataResponse;
 use OCP\AppFramework\OCSController;
@@ -26,19 +25,21 @@
 use OCP\IL10N;
 use OCP\IRequest;
 use OCP\IUserSession;
+use Psr\Log\LoggerInterface;
 
 /**
  * Class LockController
  *
  * @package OCA\FilesLock\Controller
  */
 class LockController extends OCSController {
-	use TLogger;
 
 	private int $ocsVersion;
+	private LoggerInterface $logger;
 
 	public function __construct(
 		IRequest $request,
+		LoggerInterface $logger,
 		private IUserSession $userSession,
 		private FileService $fileService,
 		private LockService $lockService,
@@ -56,6 +57,7 @@ public function __construct(
 		$this->registerResponder('xml', function ($data) {
 			return $this->buildOCSResponse('xml', $data);
 		});
+		$this->logger = $logger;
 	}
 
 
@@ -171,7 +173,7 @@ protected function fail(
 		);
 
 		if ($log) {
-			$this->log(2, $status . ' - ' . json_encode($data));
+			$this->logger->warning('[warning] ' . $status . ' - ' . json_encode($data));
 		}
 
 		return new DataResponse($data, $status);

diff --git a/lib/Db/LocksRequest.php b/lib/Db/LocksRequest.php
@@ -13,6 +13,7 @@
 use Exception;
 use OCA\FilesLock\Exceptions\LockNotFoundException;
 use OCA\FilesLock\Model\FileLock;
+use OCP\AppFramework\Utility\ITimeFactory;
 use OCP\DB\QueryBuilder\IQueryBuilder;
 
 /**
@@ -124,8 +125,10 @@ public function getAll(): array {
 	 * @throws Exception
 	 */
 	public function getLocksOlderThan(int $timeout): array {
+		$now = \OC::$server->get(ITimeFactory::class)->getTime();
+		$oldCreationTime = $now - $timeout * 60;
 		$qb = $this->getLocksSelectSql();
-		$qb->andWhere($qb->expr()->lt('l.creation', $qb->createNamedParameter($timeout * 60, IQueryBuilder::PARAM_INT)));
+		$qb->andWhere($qb->expr()->lt('l.creation', $qb->createNamedParameter($oldCreationTime, IQueryBuilder::PARAM_INT)));
 
 		return $this->getLocksFromRequest($qb);
 	}

diff --git a/lib/Service/LockService.php b/lib/Service/LockService.php
@@ -14,7 +14,6 @@
 use OCA\FilesLock\Exceptions\LockNotFoundException;
 use OCA\FilesLock\Exceptions\UnauthorizedUnlockException;
 use OCA\FilesLock\Model\FileLock;
-use OCA\FilesLock\Tools\Traits\TLogger;
 use OCA\FilesLock\Tools\Traits\TStringTools;
 use OCP\App\IAppManager;
 use OCP\EventDispatcher\IEventDispatcher;
@@ -27,14 +26,13 @@
 use OCP\IRequest;
 use OCP\IUserManager;
 use OCP\IUserSession;
+use Psr\Log\LoggerInterface;
 
 class LockService {
 	public const PREFIX = 'files_lock';
 
 
 	use TStringTools;
-	use TLogger;
-
 
 	private IUserManager $userManager;
 	private IL10N $l10n;
@@ -45,6 +43,7 @@ class LockService {
 	private IEventDispatcher $eventDispatcher;
 	private IUserSession $userSession;
 	private IRequest $request;
+	private LoggerInterface $logger;
 
 
 	private array $locks = [];
@@ -64,6 +63,7 @@ public function __construct(
 		IEventDispatcher $eventDispatcher,
 		IUserSession $userSession,
 		IRequest $request,
+		LoggerInterface $logger,
 	) {
 		$this->l10n = $l10n;
 		$this->userManager = $userManager;
@@ -74,8 +74,7 @@ public function __construct(
 		$this->eventDispatcher = $eventDispatcher;
 		$this->userSession = $userSession;
 		$this->request = $request;
-
-		$this->setup('app', 'files_lock');
+		$this->logger = $logger;
 	}
 
 	/**
@@ -153,7 +152,7 @@ public function lock(LockContext $lockScope): FileLock {
 				$known->setTimeout(
 					$known->getETA() !== FileLock::ETA_INFINITE ? $known->getTimeout() - $known->getETA() + $this->configService->getTimeoutSeconds() : 0
 				);
-				$this->notice('extending existing lock', false, ['fileLock' => $known]);
+				$this->logger->notice('extending existing lock', ['fileLock' => $known]);
 				$this->locksRequest->update($known);
 				$this->injectMetadata($known);
 				return $known;
@@ -165,7 +164,7 @@ public function lock(LockContext $lockScope): FileLock {
 			$lock = FileLock::fromLockScope($lockScope, $this->configService->getTimeoutSeconds());
 			$this->generateToken($lock);
 			$lock->setCreation(time());
-			$this->notice('locking file', false, ['fileLock' => $lock]);
+			$this->logger->notice('locking file', ['fileLock' => $lock]);
 			$this->injectMetadata($lock);
 			$this->locksRequest->save($lock);
 			$this->propagateEtag($lockScope);
@@ -189,7 +188,7 @@ public function getAppName(string $appId): ?string {
 	 * @throws UnauthorizedUnlockException
 	 */
 	public function unlock(LockContext $lock, bool $force = false): FileLock {
-		$this->notice('unlocking file', false, ['fileLock' => $lock]);
+		$this->logger->notice('unlocking file', ['fileLock' => $lock]);
 
 		$known = $this->getLockFromFileId($lock->getNode()->getId());
 		if (!$force) {
@@ -280,15 +279,16 @@ public function unlockFile(int $fileId, string $userId, bool $force = false, int
 	public function getDeprecatedLocks(): array {
 		$timeout = (int)$this->configService->getAppValue(ConfigService::LOCK_TIMEOUT);
 		if ($timeout === 0) {
-			$this->notice(
-				'ConfigService::LOCK_TIMEOUT is not numerical, using default', true, ['current' => $timeout]
+			$this->logger->notice(
+				'ConfigService::LOCK_TIMEOUT is not numerical, using default', ['current' => $timeout, 'exception' => new \Exception()]
 			);
 			$timeout = (int)$this->configService->defaults[ConfigService::LOCK_TIMEOUT];
 		}
 
 		try {
 			$locks = $this->locksRequest->getLocksOlderThan($timeout);
 		} catch (Exception $e) {
+			$this->logger->warning('Failed to get locks older then timeout', ['exception' => $e]);
 			return [];
 		}
 
@@ -376,7 +376,7 @@ function (FileLock $lock) {
 			}, $locks
 		);
 
-		$this->notice('removing locks', false, ['ids' => $ids]);
+		$this->logger->notice('removing locks', ['ids' => $ids]);
 
 		$this->locksRequest->removeIds($ids);
 	}

diff --git a/lib/Tools/Traits/TLogger.php b/lib/Tools/Traits/TLogger.php
diff --git a/tests/Feature/LockFeatureTest.php b/tests/Feature/LockFeatureTest.php
@@ -8,6 +8,7 @@
 use OCA\FilesLock\AppInfo\Application;
 use OCA\FilesLock\Model\FileLock;
 use OCA\FilesLock\Service\ConfigService;
+use OCA\FilesLock\Service\LockService;
 use OCP\AppFramework\Utility\ITimeFactory;
 use OCP\Files\File;
 use OCP\Files\IRootFolder;
@@ -204,6 +205,16 @@ public function testLockUserExpire() {
 		self::assertEquals('CCC', $file->getContent());
 	}
 
+	public function testExpiredLocksAreDeprecated() {
+		\OCP\Server::get(IConfig::class)->setAppValue(Application::APP_ID, ConfigService::LOCK_TIMEOUT, 30);
+		$file = $this->loginAndGetUserFolder(self::TEST_USER1)
+			->newFile('test-expired-lock-is-deprecated', 'AAA');
+		$this->lockManager->lock(new LockContext($file, ILock::TYPE_USER, self::TEST_USER1));
+		$this->toTheFuture(3600);
+		$deprecated = \OCP\Server::get(LockService::class)->getDeprecatedLocks();
+		self::assertNotEmpty($deprecated);
+	}
+
 	public function testLockUserInfinite() {
 		\OCP\Server::get(IConfig::class)->setAppValue(Application::APP_ID, ConfigService::LOCK_TIMEOUT, 0);
 		$file = $this->loginAndGetUserFolder(self::TEST_USER1)