From 100c6c3c75659786e5bf92c268c745f34dfdd471 Mon Sep 17 00:00:00 2001 From: Git'Fellow <12234510+solracsf@users.noreply.github.com> Date: Sun, 6 Sep 2026 19:36:39 +0200 Subject: [PATCH 1/8] feat(db): one lock per file and one absolute expiry The lock table had no uniqueness on file_id and acquisition read before it wrote, so two requests could both take a lock on the same file. Expiry existed twice over, as a ttl counted from creation for the ETA and as a creation-age query for the cleanup, and the two disagreed the moment a lock was refreshed: the job removed a lock a client believed it still held. - a unique index on file_id, with a migration that reconciles the duplicates an existing installation may already hold before it adds the constraint, and a second step for the index itself because some databases refuse two indexes on one column list in a single change - acquisition inserts and reads the winner back when the database rejects it, so the conflict comes from the constraint rather than from a prior select; a rejection on the token index is retried with a fresh token instead of being reported as a conflict on the file - expires_at is the only expiry: it drives the ETA, the refresh, the cleanup query and the validity check - the cleanup deletes only rows that are still expired when the delete runs, so a lock refreshed after the batch was read is no longer dropped Authorization is untouched here and moves to the policy in the next commit. Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com> --- lib/Cron/Unlock.php | 2 +- lib/Db/LocksRequest.php | 131 +++++--- lib/Exceptions/LockConflictException.php | 19 ++ .../Version36000Date20260906120000.php | 128 ++++++++ .../Version36000Date20260906120100.php | 35 +++ lib/Model/FileLock.php | 76 ++++- lib/Service/LockService.php | 286 +++++++++++++----- tests/Feature/AcquisitionTest.php | 184 +++++++++++ tests/Feature/ExpirationTest.php | 224 ++++++++++++++ tests/Feature/LockFeatureTest.php | 21 +- tests/Feature/MigrationTest.php | 132 ++++++++ tests/Feature/fixtures/concurrent-lock.php | 50 +++ 12 files changed, 1141 insertions(+), 147 deletions(-) create mode 100644 lib/Exceptions/LockConflictException.php create mode 100644 lib/Migration/Version36000Date20260906120000.php create mode 100644 lib/Migration/Version36000Date20260906120100.php create mode 100644 tests/Feature/AcquisitionTest.php create mode 100644 tests/Feature/ExpirationTest.php create mode 100644 tests/Feature/MigrationTest.php create mode 100644 tests/Feature/fixtures/concurrent-lock.php diff --git a/lib/Cron/Unlock.php b/lib/Cron/Unlock.php index 41f2b9d7..812980f2 100644 --- a/lib/Cron/Unlock.php +++ b/lib/Cron/Unlock.php @@ -29,6 +29,6 @@ protected function run($argument): void { } private function deleteExpiredLocks(): void { - $this->lockService->removeLocks($this->lockService->getDeprecatedLocks(1000)); + $this->lockService->removeLocksIfExpired($this->lockService->getExpiredLocks(1000)); } } diff --git a/lib/Db/LocksRequest.php b/lib/Db/LocksRequest.php index fb3c8187..d67b15f7 100644 --- a/lib/Db/LocksRequest.php +++ b/lib/Db/LocksRequest.php @@ -9,17 +9,14 @@ namespace OCA\FilesLock\Db; -use OCA\FilesLock\ConfigLexicon; use OCA\FilesLock\Cron\Unlock; +use OCA\FilesLock\Exceptions\LockConflictException; use OCA\FilesLock\Exceptions\LockNotFoundException; use OCA\FilesLock\Model\FileLock; -use OCP\AppFramework\Services\IAppConfig; -use OCP\AppFramework\Utility\ITimeFactory; use OCP\DB\Exception; use OCP\DB\IResult; use OCP\DB\QueryBuilder\IQueryBuilder; use OCP\IDBConnection; -use OCP\Server; /** * Class LocksRequest @@ -28,71 +25,128 @@ */ class LocksRequest { public const string TABLE_LOCKS = 'files_lock'; - private readonly int $timeout; + private const array COLUMNS = ['id', 'user_id', 'file_id', 'token', 'creation', 'type', 'ttl', 'owner', 'scope', 'expires_at']; public function __construct( - IAppConfig $appConfig, private readonly IDBConnection $connection, ) { - $this->timeout = $appConfig->getAppValueInt(ConfigLexicon::LOCK_TIMEOUT) * 60; } + /** + * Insert a new lock. The unique index on file_id guarantees at most one row per + * file; a violation is reported as LockConflictException so the caller can + * re-read the winning lock. + * + * @throws LockConflictException + * @throws Exception + */ public function save(FileLock $lock): void { $qb = $this->connection->getQueryBuilder(); $qb->insert(self::TABLE_LOCKS); $qb->setValue('user_id', $qb->createNamedParameter($lock->getOwner())) - ->setValue('file_id', $qb->createNamedParameter($lock->getFileId())) + ->setValue('file_id', $qb->createNamedParameter($lock->getFileId(), IQueryBuilder::PARAM_INT)) ->setValue('token', $qb->createNamedParameter($lock->getToken())) - ->setValue('creation', $qb->createNamedParameter($lock->getCreatedAt())) - ->setValue('type', $qb->createNamedParameter($lock->getType())) - ->setValue('ttl', $qb->createNamedParameter($lock->getTimeout())) - ->setValue('owner', $qb->createNamedParameter($lock->getDisplayName() ?? '')); + ->setValue('creation', $qb->createNamedParameter($lock->getCreatedAt(), IQueryBuilder::PARAM_INT)) + ->setValue('type', $qb->createNamedParameter($lock->getType(), IQueryBuilder::PARAM_INT)) + ->setValue('ttl', $qb->createNamedParameter(max(0, $lock->getTimeout()), IQueryBuilder::PARAM_INT)) + ->setValue('owner', $qb->createNamedParameter($lock->getDisplayName() ?? '')) + ->setValue('scope', $qb->createNamedParameter($lock->getScope(), IQueryBuilder::PARAM_INT)) + ->setValue('expires_at', $qb->createNamedParameter($lock->getExpiresAt(), IQueryBuilder::PARAM_INT)); try { $qb->executeStatement(); - $lock->setId($qb->getLastInsertId()); } catch (Exception $e) { if ($e->getReason() === Exception::REASON_UNIQUE_CONSTRAINT_VIOLATION) { - return; + throw new LockConflictException('A lock already exists for file ' . $lock->getFileId(), 0, $e); } throw $e; } + $lock->setId($qb->getLastInsertId()); } public function update(FileLock $lock): void { $qb = $this->connection->getQueryBuilder(); $qb->update(self::TABLE_LOCKS); $qb->set('token', $qb->createNamedParameter($lock->getToken())) - ->set('ttl', $qb->createNamedParameter($lock->getTimeout())) + ->set('ttl', $qb->createNamedParameter(max(0, $lock->getTimeout()), IQueryBuilder::PARAM_INT)) + ->set('expires_at', $qb->createNamedParameter($lock->getExpiresAt(), IQueryBuilder::PARAM_INT)) ->set('user_id', $qb->createNamedParameter($lock->getOwner())) ->set('owner', $qb->createNamedParameter($lock->getDisplayName() ?? '')) - ->set('scope', $qb->createNamedParameter($lock->getScope())) - ->where($qb->expr()->eq('id', $qb->createNamedParameter($lock->getId()))); + ->set('scope', $qb->createNamedParameter($lock->getScope(), IQueryBuilder::PARAM_INT)) + ->where($qb->expr()->eq('id', $qb->createNamedParameter($lock->getId(), IQueryBuilder::PARAM_INT))); $qb->executeStatement(); } public function delete(FileLock $lock): void { - $qb = $this->connection->getQueryBuilder(); - $qb->delete(self::TABLE_LOCKS) - ->where($qb->expr()->eq('id', $qb->createNamedParameter($lock->getId()))); - - $qb->executeStatement(); + $this->removeIds([$lock->getId()]); } /** * @param int[] $ids */ public function removeIds(array $ids): void { - if (empty($ids)) { - return; + foreach (array_chunk($ids, IQueryBuilder::MAX_IN_PARAMETERS) as $chunk) { + $qb = $this->connection->getQueryBuilder(); + $qb->delete(self::TABLE_LOCKS) + ->where($qb->expr()->in('id', $qb->createNamedParameter($chunk, IQueryBuilder::PARAM_INT_ARRAY))); + + $qb->executeStatement(); } + } + /** + * Remove the given locks, but only those that are still expired at $now. + * + * The cleanup paths read a batch of expired locks and delete it afterwards; + * in between, the owner may have refreshed one of them, which reuses the same + * row. Deleting by id alone would drop a lock that is valid again by then. + * + * @param int[] $ids + * + * @return int number of rows removed + */ + public function removeExpiredIds(array $ids, int $now): int { + $removed = 0; + foreach (array_chunk($ids, IQueryBuilder::MAX_IN_PARAMETERS) as $chunk) { + $qb = $this->connection->getQueryBuilder(); + $qb->delete(self::TABLE_LOCKS) + ->where($qb->expr()->in('id', $qb->createNamedParameter($chunk, IQueryBuilder::PARAM_INT_ARRAY))) + ->andWhere($qb->expr()->isNotNull('expires_at')) + ->andWhere($qb->expr()->lte('expires_at', $qb->createNamedParameter($now, IQueryBuilder::PARAM_INT))); + + $removed += $qb->executeStatement(); + } + + return $removed; + } + + /** + * @param list $fileIds + */ + public function removeByFileIds(array $fileIds): void { + foreach (array_chunk($fileIds, IQueryBuilder::MAX_IN_PARAMETERS) as $chunk) { + $qb = $this->connection->getQueryBuilder(); + $qb->delete(self::TABLE_LOCKS) + ->where($qb->expr()->in('file_id', $qb->createNamedParameter($chunk, IQueryBuilder::PARAM_INT_ARRAY))); + + $qb->executeStatement(); + } + } + + /** + * Remove the lock of a file if it has expired at $now. + * + * @return bool whether a row was removed + */ + public function removeExpired(int $fileId, int $now): bool { $qb = $this->connection->getQueryBuilder(); $qb->delete(self::TABLE_LOCKS) - ->where($qb->expr()->in('id', $qb->createNamedParameter($ids, IQueryBuilder::PARAM_INT_ARRAY))); + ->where($qb->expr()->eq('file_id', $qb->createNamedParameter($fileId, IQueryBuilder::PARAM_INT))) + ->andWhere($qb->expr()->isNotNull('expires_at')) + ->andWhere($qb->expr()->lte('expires_at', $qb->createNamedParameter($now, IQueryBuilder::PARAM_INT))); - $qb->executeStatement(); + return $qb->executeStatement() > 0; } /** @@ -100,9 +154,9 @@ public function removeIds(array $ids): void { */ public function getFromFileId(int $fileId): FileLock { $qb = $this->connection->getQueryBuilder(); - $qb->select('id', 'user_id', 'file_id', 'token', 'creation', 'type', 'ttl', 'owner') + $qb->select(...self::COLUMNS) ->from(self::TABLE_LOCKS) - ->where($qb->expr()->eq('file_id', $qb->createNamedParameter($fileId))); + ->where($qb->expr()->eq('file_id', $qb->createNamedParameter($fileId, IQueryBuilder::PARAM_INT))); return $this->getLockFromRequest($qb->executeQuery()); } @@ -111,11 +165,10 @@ public function getFromFileId(int $fileId): FileLock { * @param list $fileIds * * @return list - * @throws LockNotFoundException */ public function getFromFileIds(array $fileIds): array { $qb = $this->connection->getQueryBuilder(); - $qb->select('id', 'user_id', 'file_id', 'token', 'creation', 'type', 'ttl', 'owner') + $qb->select(...self::COLUMNS) ->from(self::TABLE_LOCKS) ->where($qb->expr()->in('file_id', $qb->createNamedParameter($fileIds, IQueryBuilder::PARAM_INT_ARRAY))); @@ -127,26 +180,26 @@ public function getFromFileIds(array $fileIds): array { */ public function getAll(): array { $qb = $this->connection->getQueryBuilder(); - $qb->select('id', 'user_id', 'file_id', 'token', 'creation', 'type', 'ttl', 'owner') + $qb->select(...self::COLUMNS) ->from(self::TABLE_LOCKS); return $this->getLocksFromRequest($qb->executeQuery()); } /** - * @param int $timeout in minutes + * Locks whose expiry lies at or before $now. + * * @param int $limit how many locks to retrieve (0 for all, default) * * @return list * @throws Exception */ - public function getLocksOlderThan(int $timeout, int $limit = 0): array { - $now = Server::get(ITimeFactory::class)->getTime(); - $oldCreationTime = $now - $timeout * 60; + public function getExpired(int $now, int $limit = 0): array { $qb = $this->connection->getQueryBuilder(); - $qb->select('id', 'user_id', 'file_id', 'token', 'creation', 'type', 'ttl', 'owner') + $qb->select(...self::COLUMNS) ->from(self::TABLE_LOCKS) - ->andWhere($qb->expr()->lt('creation', $qb->createNamedParameter($oldCreationTime, IQueryBuilder::PARAM_INT))); + ->where($qb->expr()->isNotNull('expires_at')) + ->andWhere($qb->expr()->lte('expires_at', $qb->createNamedParameter($now, IQueryBuilder::PARAM_INT))); if ($limit !== 0) { $qb->setMaxResults($limit); @@ -160,6 +213,7 @@ public function getLocksOlderThan(int $timeout, int $limit = 0): array { */ protected function getLockFromRequest(IResult $result): FileLock { $row = $result->fetch(); + $result->closeCursor(); if ($row === false) { throw new LockNotFoundException('Lock not found'); } @@ -175,11 +229,12 @@ public function getLocksFromRequest(IResult $result): array { while ($row = $result->fetch()) { $locks[] = $this->parseLockSelectSql($row); } + $result->closeCursor(); return $locks; } public function parseLockSelectSql(array $data): FileLock { - $lock = new FileLock($this->timeout); + $lock = new FileLock(); $lock->importFromDatabase($data); return $lock; diff --git a/lib/Exceptions/LockConflictException.php b/lib/Exceptions/LockConflictException.php new file mode 100644 index 00000000..e04c1808 --- /dev/null +++ b/lib/Exceptions/LockConflictException.php @@ -0,0 +1,19 @@ +hasTable(LocksRequest::TABLE_LOCKS)) { + return; + } + if ($schema->getTable(LocksRequest::TABLE_LOCKS)->hasIndex(self::INDEX_FILE_ID)) { + return; + } + + $removed = $this->removeDuplicateLocks(); + if ($removed > 0) { + $output->info('Removed ' . $removed . ' superseded duplicate file lock rows'); + } + } + + #[\Override] + public function changeSchema(IOutput $output, Closure $schemaClosure, array $options): ?ISchemaWrapper { + /** @var ISchemaWrapper $schema */ + $schema = $schemaClosure(); + $table = $schema->getTable(LocksRequest::TABLE_LOCKS); + $changed = false; + + if (!$table->hasColumn('expires_at')) { + $table->addColumn('expires_at', Types::BIGINT, [ + 'notnull' => false, + 'default' => null, + ]); + $changed = true; + } + + // the unique index replacing it is created by the next migration step, + // some databases refuse a second index on the same column list + foreach ($table->getIndexes() as $index) { + if ($index->isSimpleIndex() && $index->spansColumns(['file_id'])) { + $table->dropIndex($index->getName()); + $changed = true; + } + } + + if (!$table->hasIndex(self::INDEX_EXPIRES)) { + $table->addIndex(['expires_at'], self::INDEX_EXPIRES); + $changed = true; + } + + return $changed ? $schema : null; + } + + #[\Override] + public function postSchemaChange(IOutput $output, Closure $schemaClosure, array $options): void { + $qb = $this->connection->getQueryBuilder(); + $qb->update(LocksRequest::TABLE_LOCKS) + ->set('expires_at', $qb->func()->add('creation', 'ttl')) + ->where($qb->expr()->isNull('expires_at')) + ->andWhere($qb->expr()->gt('ttl', $qb->createNamedParameter(0, IQueryBuilder::PARAM_INT))); + $qb->executeStatement(); + } + + /** + * Keep the row with the highest id for every file that has more than one lock. + * + * @return int number of rows removed + */ + public function removeDuplicateLocks(): int { + $qb = $this->connection->getQueryBuilder(); + $qb->select('file_id') + ->selectAlias($qb->func()->max('id'), 'keep_id') + ->selectAlias($qb->func()->count('id'), 'lock_count') + ->from(LocksRequest::TABLE_LOCKS) + ->groupBy('file_id') + ->having($qb->expr()->gt($qb->func()->count('id'), $qb->createNamedParameter(1, IQueryBuilder::PARAM_INT))); + + $result = $qb->executeQuery(); + $duplicates = []; + while ($row = $result->fetch()) { + $duplicates[(int)$row['file_id']] = (int)$row['keep_id']; + } + $result->closeCursor(); + + $removed = 0; + foreach ($duplicates as $fileId => $keepId) { + $delete = $this->connection->getQueryBuilder(); + $delete->delete(LocksRequest::TABLE_LOCKS) + ->where($delete->expr()->eq('file_id', $delete->createNamedParameter($fileId, IQueryBuilder::PARAM_INT))) + ->andWhere($delete->expr()->neq('id', $delete->createNamedParameter($keepId, IQueryBuilder::PARAM_INT))); + $removed += $delete->executeStatement(); + } + + return $removed; + } +} diff --git a/lib/Migration/Version36000Date20260906120100.php b/lib/Migration/Version36000Date20260906120100.php new file mode 100644 index 00000000..31cf143c --- /dev/null +++ b/lib/Migration/Version36000Date20260906120100.php @@ -0,0 +1,35 @@ +getTable(LocksRequest::TABLE_LOCKS); + if ($table->hasIndex(Version36000Date20260906120000::INDEX_FILE_ID)) { + return null; + } + + $table->addUniqueIndex(['file_id'], Version36000Date20260906120000::INDEX_FILE_ID); + return $schema; + } +} diff --git a/lib/Model/FileLock.php b/lib/Model/FileLock.php index c9c7c16a..c95d06f1 100644 --- a/lib/Model/FileLock.php +++ b/lib/Model/FileLock.php @@ -19,6 +19,9 @@ /** * Class FileLock * + * Expiry is modelled by a single absolute timestamp: null means the lock never + * expires, any other value is the unix time at which it stops being valid. + * * @package OCA\FilesLock\Service */ class FileLock implements ILock, JsonSerializable { @@ -36,25 +39,26 @@ class FileLock implements ILock, JsonSerializable { private int $creation = 0; + private ?int $expiresAt = null; + private int $lockType = ILock::TYPE_USER; private ?string $displayName = null; private int $scope = ILock::LOCK_EXCLUSIVE; - /** - * FileLock constructor. - */ - public function __construct( - private int $timeout = 1800, - ) { + public function __construct() { $this->creation = Server::get(ITimeFactory::class)->getTime(); } + /** + * @param int $timeout lifetime in seconds counted from creation, <= 0 for a lock that never expires + */ public static function fromLockScope(LockContext $lockScope, int $timeout): FileLock { - $lock = new FileLock($timeout); + $lock = new FileLock(); $lock->setUserId($lockScope->getOwner()); $lock->setLockType($lockScope->getType()); $lock->setFileId($lockScope->getNode()->getId()); + $lock->setTimeout($timeout); return $lock; } @@ -111,23 +115,52 @@ public function setToken(string $token): self { return $this; } + /** + * Lifetime of the lock in seconds counted from its creation, ETA_INFINITE when it never expires. + */ #[\Override] public function getTimeout(): int { - return $this->timeout; + if ($this->expiresAt === null) { + return self::ETA_INFINITE; + } + return max(0, $this->expiresAt - $this->creation); } + /** + * @param int $timeout lifetime in seconds counted from creation, <= 0 for a lock that never expires + */ public function setTimeout(int $timeout): self { - $this->timeout = $timeout; + $this->expiresAt = $timeout > 0 ? $this->creation + $timeout : null; return $this; } + public function getExpiresAt(): ?int { + return $this->expiresAt; + } + + public function setExpiresAt(?int $expiresAt): self { + $this->expiresAt = $expiresAt; + + return $this; + } + + public function isInfinite(): bool { + return $this->expiresAt === null; + } + + public function isExpired(int $now): bool { + return $this->expiresAt !== null && $this->expiresAt <= $now; + } + + /** + * Seconds until the lock expires, 0 when it is already expired, ETA_INFINITE when it never expires. + */ public function getETA(): int { - if ($this->getTimeout() <= 0) { + if ($this->expiresAt === null) { return self::ETA_INFINITE; } - $end = $this->getCreatedAt() + $this->getTimeout(); - $eta = $end - Server::get(ITimeFactory::class)->getTime(); + $eta = $this->expiresAt - Server::get(ITimeFactory::class)->getTime(); return ($eta < 1) ? 0 : $eta; } @@ -181,10 +214,10 @@ public function toLockInfo(): LockInfo { $lock = new LockInfo(); $lock->owner = $this->getDisplayName(); $lock->token = $this->getToken(); - $lock->timeout = $this->getTimeout() <= 0 ? LockInfo::TIMEOUT_INFINITE : $this->getTimeout(); + $lock->timeout = $this->isInfinite() ? LockInfo::TIMEOUT_INFINITE : $this->getETA(); $lock->created = $this->getCreatedAt(); $lock->scope = LockInfo::EXCLUSIVE; - $lock->depth = 1; + $lock->depth = 0; $lock->uri = $this->getUri(); return $lock; @@ -197,8 +230,12 @@ public function importFromDatabase(array $data): self { $this->setToken($data['token'] ?? ''); $this->setCreation((int)$data['creation']); $this->setLockType((int)$data['type']); - $this->setTimeout((int)$data['ttl']); + $this->setExpiresAt(isset($data['expires_at']) ? (int)$data['expires_at'] : null); $this->setDisplayName($data['owner'] ?? ''); + // rows written before the scope was persisted on insert hold 0, which is + // not a valid scope; those locks are exclusive like every other one + $scope = (int)($data['scope'] ?? 0); + $this->setScope(in_array($scope, [ILock::LOCK_EXCLUSIVE, ILock::LOCK_SHARED], true) ? $scope : ILock::LOCK_EXCLUSIVE); return $this; } @@ -214,7 +251,13 @@ public function import(array $data): void { $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)); + if (array_key_exists('expiresAt', $data)) { + $this->setExpiresAt($data['expiresAt'] === null ? null : (int)$data['expiresAt']); + } elseif (array_key_exists('expires_at', $data)) { + $this->setExpiresAt($data['expires_at'] === null ? null : (int)$data['expires_at']); + } elseif (isset($data['ttl'])) { + $this->setTimeout((int)$data['ttl']); + } $this->setDisplayName((string)($data['displayName'] ?? $data['owner'] ?? '')); } @@ -229,6 +272,7 @@ public function jsonSerialize(): array { 'token' => $this->getToken(), 'eta' => $this->getETA(), 'creation' => $this->getCreatedAt(), + 'expiresAt' => $this->getExpiresAt(), 'type' => $this->getType(), ]; } diff --git a/lib/Service/LockService.php b/lib/Service/LockService.php index cbc643c6..71a8a604 100644 --- a/lib/Service/LockService.php +++ b/lib/Service/LockService.php @@ -15,13 +15,16 @@ use OCA\FilesLock\AppInfo\Application; use OCA\FilesLock\ConfigLexicon; use OCA\FilesLock\Db\LocksRequest; +use OCA\FilesLock\Exceptions\LockConflictException; use OCA\FilesLock\Exceptions\LockNotFoundException; use OCA\FilesLock\Exceptions\UnauthorizedUnlockException; use OCA\FilesLock\Model\FileLock; use OCP\App\IAppManager; use OCP\AppFramework\Services\IAppConfig; +use OCP\AppFramework\Utility\ITimeFactory; use OCP\Constants; use OCP\EventDispatcher\IEventDispatcher; +use OCP\Files\IHomeStorage; use OCP\Files\InvalidPathException; use OCP\Files\IRootFolder; use OCP\Files\Lock\ILock; @@ -33,11 +36,14 @@ use OCP\IRequest; use OCP\IUserManager; use OCP\IUserSession; +use OCP\Server; use Psr\Log\LoggerInterface; class LockService { public const PREFIX = 'files_lock'; + /** @var array */ private array $lockCache = []; + /** @var array */ private array $remoteLockCache = []; private bool $allowUserOverride = false; @@ -56,29 +62,50 @@ public function __construct( ) { } + /** + * Resolved lazily so that a clock replaced in the container after this + * service was built (tests) is honoured everywhere. + */ + private function now(): int { + return Server::get(ITimeFactory::class)->getTime(); + } + public function clearCache(): void { $this->lockCache = []; $this->remoteLockCache = []; } + /** + * Active local lock of a file, using the per-request cache. Never consults remote storages. + */ + public function getActiveLock(int $fileId): ?FileLock { + if (array_key_exists($fileId, $this->lockCache)) { + $cached = $this->lockCache[$fileId]; + if ($cached instanceof FileLock && $cached->isExpired($this->now())) { + $this->locksRequest->removeExpired($fileId, $this->now()); + $this->lockCache[$fileId] = false; + return null; + } + return $cached ?: null; + } + + try { + return $this->getLockFromFileId($fileId); + } catch (LockNotFoundException) { + return null; + } + } + 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]; + $lock = $this->getActiveLock($nodeId); + if ($lock !== null) { + return $lock; } if (array_key_exists($nodeId, $this->remoteLockCache)) { return $this->remoteLockCache[$nodeId]; } - if (!array_key_exists($nodeId, $this->lockCache)) { - try { - $this->lockCache[$nodeId] = $this->getLockFromFileId($nodeId); - return $this->lockCache[$nodeId]; - } catch (LockNotFoundException) { - $this->lockCache[$nodeId] = false; - } - } - $remoteLock = $this->getRemoteLockFromDav($nodeId, $node); $this->remoteLockCache[$nodeId] = $remoteLock ?: false; return $this->remoteLockCache[$nodeId]; @@ -93,6 +120,7 @@ public function getLockForNodeIds(array $nodeIds): array { $locks = []; $locksToRequest = []; foreach ($nodeIds as $nodeId) { + $nodeId = (int)$nodeId; if (array_key_exists($nodeId, $this->lockCache) && $this->lockCache[$nodeId] instanceof FileLock) { $locks[$nodeId] = $this->lockCache[$nodeId]; } elseif (array_key_exists($nodeId, $this->remoteLockCache)) { @@ -117,9 +145,10 @@ public function getLockForNodeIds(array $nodeIds): array { } $newLocks = array_merge(...$newLocks); + $now = $this->now(); $expiredLocks = []; foreach ($newLocks as $lock) { - if ($lock->getETA() === 0) { + if ($lock->isExpired($now)) { $expiredLocks[] = $lock->getId(); $locks[$lock->getFileId()] = false; $this->lockCache[$lock->getFileId()] = false; @@ -130,52 +159,101 @@ public function getLockForNodeIds(array $nodeIds): array { } if (count($expiredLocks) > 0) { - $this->locksRequest->removeIds($expiredLocks); + $this->locksRequest->removeExpiredIds($expiredLocks, $now); } return $locks; } + /** + * Configured lock lifetime in seconds, ETA_INFINITE when locks never expire. + */ + public function getConfiguredTimeout(): int { + $minutes = $this->appConfig->getAppValueInt(ConfigLexicon::LOCK_TIMEOUT); + return $minutes > 0 ? $minutes * 60 : FileLock::ETA_INFINITE; + } + public function lock(LockContext $lockScope): FileLock { + return $this->acquire($lockScope); + } + + /** + * Acquire or refresh the lock described by $lockScope. + * + * The database enforces at most one lock per file; a lost race is reported as + * OwnerLockedException carrying the winning lock. + * + * @param int|null $timeout lifetime in seconds, <= 0 for no expiry, null for the configured value + * @param string|null $token token to record for a new lock (native WebDAV), generated when null + * @param string|null $displayName display name to record, resolved from the owner when null + * + * @throws OwnerLockedException + * @throws UnauthorizedUnlockException + */ + public function acquire(LockContext $lockScope, ?int $timeout = null, ?string $token = null, ?string $displayName = null): FileLock { $this->canLock($lockScope); - $timeout = $this->appConfig->getAppValueInt(ConfigLexicon::LOCK_TIMEOUT) * 60; + $fileId = $lockScope->getNode()->getId(); + $timeout ??= $this->getConfiguredTimeout(); + $now = $this->now(); - try { - $known = $this->getLockFromFileId($lockScope->getNode()->getId()); - - // Extend lock expiry if matching - if ( - $known->getType() === $lockScope->getType() && ($known->getOwner() === $lockScope->getOwner() || $known->getToken() === $lockScope->getOwner()) - ) { - $known->setTimeout($known->getETA() !== FileLock::ETA_INFINITE ? $known->getTimeout() - $known->getETA() + $timeout : 0); - $this->logger->notice('extending existing lock', ['fileLock' => $known]); - $this->locksRequest->update($known); - $this->lockCache[$lockScope->getNode()->getId()] = $known; - $this->injectMetadata($known); - return $known; - } + $this->locksRequest->removeExpired($fileId, $now); + unset($this->lockCache[$fileId]); - $this->injectMetadata($known); - throw new OwnerLockedException($known); + try { + $known = $this->locksRequest->getFromFileId($fileId); + return $this->refreshOrConflict($known, $lockScope, $timeout, $now); } catch (LockNotFoundException) { - $lock = FileLock::fromLockScope($lockScope, $timeout); - $this->generateToken($lock); - $this->logger->notice('locking file', ['fileLock' => $lock]); + } + + $lock = FileLock::fromLockScope($lockScope, 0); + $lock->setCreation($now); + $lock->setExpiresAt($timeout > 0 ? $now + $timeout : null); + $lock->setToken($token ?? self::PREFIX . '/' . uuid_create(UUID_TYPE_RANDOM)); + if ($displayName !== null) { + $lock->setDisplayName($displayName); + } else { $this->injectMetadata($lock); + } + + try { $this->locksRequest->save($lock); - $this->propagateEtag($lockScope); - return $lock; + } catch (LockConflictException) { + $known = null; + try { + $known = $this->locksRequest->getFromFileId($fileId); + } catch (LockNotFoundException) { + // no row for this file, so the unique index that fired was the one on + // the token; take a fresh token and try once more + $lock->setToken(self::PREFIX . '/' . uuid_create(UUID_TYPE_RANDOM)); + $this->locksRequest->save($lock); + } + if ($known !== null) { + return $this->refreshOrConflict($known, $lockScope, $timeout, $now); + } } - } - public function update(FileLock $lock): void { - $this->locksRequest->update($lock); + $this->logger->notice('locking file', ['fileLock' => $lock]); + $this->lockCache[$fileId] = $lock; + $this->propagateEtag($lockScope->getNode()); + return $lock; } - public function getAppName(string $appId): ?string { - /** @var array{name: null}|null $appInfo */ - $appInfo = $this->appManager->getAppInfo($appId); - return $appInfo['name'] ?? null; + /** + * @throws OwnerLockedException + */ + private function refreshOrConflict(FileLock $known, LockContext $lockScope, int $timeout, int $now): FileLock { + $this->injectMetadata($known); + if (!($known->getType() === $lockScope->getType() + && ($known->getOwner() === $lockScope->getOwner() || $known->getToken() === $lockScope->getOwner()))) { + $this->lockCache[$known->getFileId()] = $known; + throw new OwnerLockedException($known); + } + + $known->setExpiresAt($timeout > 0 ? $now + $timeout : null); + $this->logger->notice('extending existing lock', ['fileLock' => $known]); + $this->locksRequest->update($known); + $this->lockCache[$known->getFileId()] = $known; + return $known; } /** @@ -194,7 +272,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; } @@ -203,14 +281,6 @@ public function enableUserOverride(): void { $this->allowUserOverride = true; } - public function canLock(LockContext $request, ?FileLock $current = null): void { - if (($request->getNode()->getPermissions() & Constants::PERMISSION_UPDATE) === 0) { - throw new UnauthorizedUnlockException( - $this->l10n->t('File can only be locked with update permissions.') - ); - } - } - public function canUnlock(LockContext $request, FileLock $current): void { $isSameUser = $current->getOwner() === $this->userSession->getUser()?->getUID(); $isSameToken = $request->getOwner() === $current->getToken(); @@ -278,41 +348,76 @@ public function unlockFile(int $fileId, ?string $userId, bool $force = false, in $lockType, $userId, ); - $this->propagateEtag($lock); + $this->propagateEtag($lock->getNode()); return $this->unlock($lock, $force); } + public function update(FileLock $lock): void { + $this->locksRequest->update($lock); + $this->lockCache[$lock->getFileId()] = $lock; + } + + public function getAppName(string $appId): ?string { + /** @var array{name: null}|null $appInfo */ + $appInfo = $this->appManager->getAppInfo($appId); + return $appInfo['name'] ?? null; + } + /** + * @throws UnauthorizedUnlockException when the node cannot be locked by the caller + * @throws NotFileException when the node is not a file + */ + public function canLock(LockContext $request, ?FileLock $current = null): void { + if (($request->getNode()->getPermissions() & Constants::PERMISSION_UPDATE) === 0) { + throw new UnauthorizedUnlockException( + $this->l10n->t('File can only be locked with update permissions.') + ); + } + } + + /** + * Locks whose expiry has passed. + * * @param int $limit how many locks to retrieve (0 for all, default) * * @return FileLock[] */ - public function getDeprecatedLocks(int $limit = 0): array { - $timeout = $this->appConfig->getAppValueInt(ConfigLexicon::LOCK_TIMEOUT); - if ($timeout === FileLock::ETA_INFINITE) { - return []; - } - + public function getExpiredLocks(int $limit = 0): array { try { - $locks = $this->locksRequest->getLocksOlderThan($timeout, $limit); + return $this->locksRequest->getExpired($this->now(), $limit); } catch (Exception $e) { - $this->logger->warning('Failed to get locks older then timeout', ['exception' => $e]); + $this->logger->warning('Failed to get expired locks', ['exception' => $e]); return []; } + } - return $locks; + /** + * @deprecated use getExpiredLocks() + * @return FileLock[] + */ + public function getDeprecatedLocks(int $limit = 0): array { + return $this->getExpiredLocks($limit); } /** + * Active lock of a file, removing it first when it has expired. + * * @throws LockNotFoundException */ public function getLockFromFileId(int $fileId): FileLock { - $lock = $this->locksRequest->getFromFileId($fileId); - if ($lock->getETA() === 0) { + try { + $lock = $this->locksRequest->getFromFileId($fileId); + } catch (LockNotFoundException $e) { + $this->lockCache[$fileId] = false; + throw $e; + } + if ($lock->isExpired($this->now())) { $this->locksRequest->delete($lock); + $this->lockCache[$fileId] = false; throw new LockNotFoundException('lock is ignored and deleted as being too old.'); } + $this->lockCache[$fileId] = $lock; return $lock; } @@ -325,11 +430,12 @@ public function injectMetadata(FileLock $lock): FileLock { $displayName = $this->getAppName($lock->getOwner()) ?? null; } if ($lock->getType() === ILock::TYPE_TOKEN) { - $clientHint = $this->getClientHint(); - $displayName = $lock->getDisplayName() ?: ( - $this->userManager->getDisplayName($lock->getOwner()) . ' ' - . ($clientHint ? ('(' . $clientHint . ')') : '') - ); + $displayName = $lock->getDisplayName(); + if ($displayName === null || $displayName === '') { + $clientHint = $this->getClientHint(); + $displayName = trim(($this->userManager->getDisplayName($lock->getOwner()) ?? $lock->getOwner()) + . ($clientHint ? (' (' . $clientHint . ')') : '')); + } } if ($displayName) { @@ -362,6 +468,26 @@ public function generateToken(FileLock $lock): void { $lock->setToken(self::PREFIX . '/' . uuid_create(UUID_TYPE_RANDOM)); } + /** + * Remove the given locks, skipping any that are no longer expired because + * their owner refreshed them after the batch was read. + * + * @param FileLock[] $locks + */ + public function removeLocksIfExpired(array $locks): void { + if (empty($locks)) { + return; + } + + $ids = array_map(fn (FileLock $lock): int => $lock->getId(), $locks); + $removed = $this->locksRequest->removeExpiredIds($ids, $this->now()); + $this->logger->notice('removing expired locks', ['candidates' => count($ids), 'removed' => $removed]); + + foreach ($locks as $lock) { + unset($this->lockCache[$lock->getFileId()]); + } + } + /** * @param FileLock[] $locks */ @@ -377,6 +503,9 @@ public function removeLocks(array $locks): void { $this->logger->notice('removing locks', ['ids' => $ids]); $this->locksRequest->removeIds($ids); + foreach ($locks as $lock) { + $this->lockCache[$lock->getFileId()] = false; + } } public function getRemoteLockFromDav(int $nodeId, ?Node $node = null): ?FileLock { @@ -404,13 +533,13 @@ public function getRemoteLockFromDav(int $nodeId, ?Node $node = null): ?FileLock return null; } - $path = $node->getInternalPath(); - $storage->getMetaData($path); - if (!method_exists($storage, 'getPropfindPropertyValue')) { return null; } + $path = $node->getInternalPath(); + $storage->getMetaData($path); + $isLocked = $storage->getPropfindPropertyValue($path, Application::DAV_PROPERTY_LOCK); if (!$isLocked) { return null; @@ -436,11 +565,14 @@ public function getRemoteLockFromDav(int $nodeId, ?Node $node = null): ?FileLock } } - private function propagateEtag(LockContext $lockContext): void { - $node = $lockContext->getNode(); - $node->getStorage()->getCache()->update($node->getId(), [ - 'etag' => uniqid(), - ]); - $node->getStorage()->getUpdater()->propagate($node->getInternalPath(), $node->getMTime()); + private function propagateEtag(Node $node): void { + try { + $node->getStorage()->getCache()->update($node->getId(), [ + 'etag' => uniqid(), + ]); + $node->getStorage()->getUpdater()->propagate($node->getInternalPath(), $node->getMTime()); + } catch (Exception $e) { + $this->logger->debug('Failed to propagate etag after lock change: ' . $e->getMessage(), ['exception' => $e]); + } } } diff --git a/tests/Feature/AcquisitionTest.php b/tests/Feature/AcquisitionTest.php new file mode 100644 index 00000000..dc72e848 --- /dev/null +++ b/tests/Feature/AcquisitionTest.php @@ -0,0 +1,184 @@ +loginAndGetUserFolder(self::USER1)->newFile('dup.txt', 'AAA'); + $request = \OCP\Server::get(LocksRequest::class); + + $first = FileLock::fromLockScope(new LockContext($file, ILock::TYPE_USER, self::USER1), 0); + $first->setToken('files_lock/dup-1'); + $request->save($first); + self::assertGreaterThan(0, $first->getId()); + + $second = FileLock::fromLockScope(new LockContext($file, ILock::TYPE_USER, self::USER2), 0); + $second->setToken('files_lock/dup-2'); + try { + $request->save($second); + self::fail('second row for the same file must be rejected'); + } catch (LockConflictException) { + } + self::assertSame(1, $this->lockRowCount($file->getId())); + } + + public function testConflictReportsWinningLock(): void { + $file = $this->sharedFile('conflict.txt'); + $mine = $this->lockManager->lock(new LockContext($file, ILock::TYPE_USER, self::USER1)); + + $shared = $this->loginAndGetUserFolder(self::USER2)->get('conflict.txt'); + try { + $this->lockManager->lock(new LockContext($shared, ILock::TYPE_USER, self::USER2)); + self::fail('expected OwnerLockedException'); + } catch (OwnerLockedException $e) { + self::assertSame($mine->getId(), $e->getLock()->getId()); + self::assertSame(self::USER1, $e->getLock()->getOwner()); + } + self::assertSame(1, $this->lockRowCount($file->getId())); + } + + public function testRefreshKeepsTheSameRow(): void { + $file = $this->loginAndGetUserFolder(self::USER1)->newFile('refresh.txt', 'AAA'); + $first = $this->lockManager->lock(new LockContext($file, ILock::TYPE_USER, self::USER1)); + $second = $this->lockManager->lock(new LockContext($file, ILock::TYPE_USER, self::USER1)); + self::assertSame($first->getId(), $second->getId()); + self::assertSame($first->getToken(), $second->getToken()); + self::assertSame(1, $this->lockRowCount($file->getId())); + } + + public function testExpiredLockIsReplaced(): void { + $this->setLockTimeoutMinutes(10); + $file = $this->sharedFile('expired.txt'); + $old = $this->lockManager->lock(new LockContext($file, ILock::TYPE_USER, self::USER1)); + $this->toTheFuture(11 * 60); + + $shared = $this->loginAndGetUserFolder(self::USER2)->get('expired.txt'); + $new = $this->lockManager->lock(new LockContext($shared, ILock::TYPE_USER, self::USER2)); + self::assertNotSame($old->getId(), $new->getId()); + self::assertSame(self::USER2, $new->getOwner()); + self::assertSame(1, $this->lockRowCount($file->getId())); + } + + public function testLockingNeedsUpdatePermission(): void { + $file = $this->sharedFile('readonly.txt', 1); + $shared = $this->loginAndGetUserFolder(self::USER2)->get('readonly.txt'); + try { + $this->lockManager->lock(new LockContext($shared, ILock::TYPE_USER, self::USER2)); + self::fail('expected UnauthorizedUnlockException'); + } catch (UnauthorizedUnlockException) { + } + self::assertSame(0, $this->lockRowCount($file->getId())); + } + + /** + * Two independent PHP processes race for the same file: exactly one wins, + * the other gets a conflict, and the table holds exactly one row. + */ + public function testConcurrentAcquisitionYieldsOneLock(): void { + // the child processes boot a normal server, so the users must live in the database backend + $userManager = \OCP\Server::get(IUserManager::class); + $databaseBackend = new \OC\User\Database(); + $userManager->registerBackend($databaseBackend); + foreach ([self::RACE_USER1, self::RACE_USER2] as $uid) { + if (!$databaseBackend->userExists($uid)) { + $databaseBackend->createUser($uid, 'race-password-' . $uid); + } + } + + try { + $file = $this->loginAndGetUserFolder(self::RACE_USER1)->newFile('race.txt', 'AAA'); + $shareManager = \OCP\Server::get(IShareManager::class); + $share = $shareManager->newShare(); + $share->setNode($file)->setSharedBy(self::RACE_USER1)->setSharedWith(self::RACE_USER2) + ->setShareType(IShare::TYPE_USER)->setPermissions(19); + $share = $shareManager->createShare($share); + $share->setStatus(IShare::STATUS_ACCEPTED); + $shareManager->updateShare($share); + + $script = __DIR__ . '/fixtures/concurrent-lock.php'; + $locked = $conflicts = 0; + for ($round = 0; $round < self::RACE_ROUNDS; $round++) { + $this->clearLocks(); + $startAt = microtime(true) + 3; + $outputs = $this->runConcurrently([ + [PHP_BINARY, $script, self::RACE_USER1, (string)$file->getId(), (string)$startAt], + [PHP_BINARY, $script, self::RACE_USER2, (string)$file->getId(), (string)$startAt], + ]); + $results = array_map(trim(...), $outputs); + sort($results); + self::assertSame(['CONFLICT', 'LOCKED'], $results, 'round ' . $round . ': ' . implode(' | ', $results)); + self::assertSame(1, $this->lockRowCount($file->getId()), 'round ' . $round); + $locked += count(array_keys($results, 'LOCKED', true)); + $conflicts += count(array_keys($results, 'CONFLICT', true)); + } + self::assertSame(self::RACE_ROUNDS, $locked); + self::assertSame(self::RACE_ROUNDS, $conflicts); + } finally { + $this->clearLocks(); + foreach ([self::RACE_USER1, self::RACE_USER2] as $uid) { + $userManager->get($uid)?->delete(); + $databaseBackend->deleteUser($uid); + } + $userManager->removeBackend($databaseBackend); + } + } + + /** + * @param list> $commands + * @return list stdout of each process + */ + private function runConcurrently(array $commands): array { + $processes = []; + $pipes = []; + foreach ($commands as $i => $command) { + $descriptors = [0 => ['pipe', 'r'], 1 => ['pipe', 'w'], 2 => ['pipe', 'w']]; + $process = proc_open($command, $descriptors, $procPipes, \OC::$SERVERROOT); + self::assertIsResource($process, 'failed to start child process'); + fclose($procPipes[0]); + $processes[$i] = $process; + $pipes[$i] = $procPipes; + } + + $outputs = []; + foreach ($processes as $i => $process) { + $stdout = stream_get_contents($pipes[$i][1]); + $stderr = stream_get_contents($pipes[$i][2]); + fclose($pipes[$i][1]); + fclose($pipes[$i][2]); + $exitCode = proc_close($process); + if ($exitCode !== 0) { + self::fail('child process failed (' . $exitCode . '): ' . $stdout . ' ' . $stderr); + } + $outputs[] = $stdout; + } + + return $outputs; + } +} diff --git a/tests/Feature/ExpirationTest.php b/tests/Feature/ExpirationTest.php new file mode 100644 index 00000000..a37052f5 --- /dev/null +++ b/tests/Feature/ExpirationTest.php @@ -0,0 +1,224 @@ +invoke($job, null); + } + + public function testFiniteLockHasAbsoluteExpiry(): void { + $this->setLockTimeoutMinutes(15); + $this->toTheFuture(0); + $file = $this->loginAndGetUserFolder(self::USER1)->newFile('finite.txt', 'AAA'); + $lock = $this->lockManager->lock(new LockContext($file, ILock::TYPE_USER, self::USER1)); + + self::assertSame($this->time + 15 * 60, $lock->getExpiresAt()); + self::assertSame(15 * 60, $lock->getTimeout()); + self::assertSame(15 * 60, $lock->getETA()); + self::assertSame($this->time + 15 * 60, $this->storedLock($file->getId())?->getExpiresAt()); + } + + public function testInfiniteLock(): void { + $this->setLockTimeoutMinutes(-1); + $file = $this->loginAndGetUserFolder(self::USER1)->newFile('infinite.txt', 'AAA'); + $lock = $this->lockManager->lock(new LockContext($file, ILock::TYPE_USER, self::USER1)); + + self::assertNull($lock->getExpiresAt()); + self::assertSame(FileLock::ETA_INFINITE, $lock->getETA()); + self::assertSame(FileLock::ETA_INFINITE, $lock->getTimeout()); + $this->toTheFuture(365 * 24 * 3600); + self::assertCount(1, $this->lockManager->getLocks($file->getId())); + self::assertSame([], $this->lockService()->getExpiredLocks()); + } + + public function testRefreshMovesExpiry(): void { + $this->setLockTimeoutMinutes(30); + $this->toTheFuture(0); + $file = $this->loginAndGetUserFolder(self::USER1)->newFile('refresh.txt', 'AAA'); + $this->lockManager->lock(new LockContext($file, ILock::TYPE_USER, self::USER1)); + + $this->toTheFuture(20 * 60); + $refreshed = $this->lockManager->lock(new LockContext($file, ILock::TYPE_USER, self::USER1)); + self::assertSame($this->time + 30 * 60, $refreshed->getExpiresAt()); + self::assertSame(30 * 60, $refreshed->getETA()); + self::assertSame(50 * 60, $refreshed->getTimeout(), 'lifetime counted from creation'); + self::assertSame($this->time + 30 * 60, $this->storedLock($file->getId())?->getExpiresAt()); + } + + public function testCronKeepsRefreshedLockAndRemovesExpiredOne(): void { + $this->setLockTimeoutMinutes(30); + $this->toTheFuture(0); + $file = $this->loginAndGetUserFolder(self::USER1)->newFile('cron.txt', 'AAA'); + $this->lockManager->lock(new LockContext($file, ILock::TYPE_USER, self::USER1)); + + $this->toTheFuture(20 * 60); + $this->lockManager->lock(new LockContext($file, ILock::TYPE_USER, self::USER1)); + + $this->toTheFuture(11 * 60); + self::assertSame([], $this->lockService()->getExpiredLocks(), 'a refreshed lock is not expired 31 minutes after creation'); + $this->runCron(); + self::assertSame(1, $this->lockRowCount($file->getId())); + + $this->toTheFuture(20 * 60); + self::assertCount(1, $this->lockService()->getExpiredLocks()); + $this->runCron(); + self::assertSame(0, $this->lockRowCount($file->getId())); + } + + public function testExpiredLockNoLongerBlocksAndIsRemovedOnRead(): void { + $this->setLockTimeoutMinutes(30); + $file = $this->sharedFile('expire-write.txt'); + $this->lockManager->lock(new LockContext($file, ILock::TYPE_USER, self::USER1)); + + $shared = $this->loginAndGetUserFolder(self::USER2)->get('expire-write.txt'); + try { + $shared->putContent('BBB'); + self::fail('lock should block before expiry'); + } catch (ManuallyLockedException) { + } + + $this->toTheFuture(31 * 60); + self::assertSame([], $this->lockManager->getLocks($file->getId())); + $shared->putContent('CCC'); + self::assertSame('CCC', $shared->getContent()); + self::assertSame(0, $this->lockRowCount($file->getId())); + } + + public function testRefreshAfterExpiryCreatesANewLock(): void { + $this->setLockTimeoutMinutes(10); + $this->toTheFuture(0); + $file = $this->loginAndGetUserFolder(self::USER1)->newFile('late.txt', 'AAA'); + $first = $this->lockManager->lock(new LockContext($file, ILock::TYPE_USER, self::USER1)); + + $this->toTheFuture(11 * 60); + $second = $this->lockManager->lock(new LockContext($file, ILock::TYPE_USER, self::USER1)); + self::assertNotSame($first->getId(), $second->getId()); + self::assertSame($this->time + 10 * 60, $second->getExpiresAt()); + } + + public function testRefreshRacingCronSurvives(): void { + $this->setLockTimeoutMinutes(30); + $this->toTheFuture(0); + $file = $this->loginAndGetUserFolder(self::USER1)->newFile('race-cron.txt', 'AAA'); + $this->lockManager->lock(new LockContext($file, ILock::TYPE_USER, self::USER1)); + + // cron selects its batch at t+29, the client refreshes at the same moment, cron deletes afterwards + $this->toTheFuture(29 * 60); + $batch = $this->lockService()->getExpiredLocks(1000); + self::assertSame([], $batch); + $this->lockManager->lock(new LockContext($file, ILock::TYPE_USER, self::USER1)); + $this->lockService()->removeLocks($batch); + self::assertSame(1, $this->lockRowCount($file->getId())); + + // a refresh that lands after cron selected an expired batch replaces the expired row, + // and cron's deletion by id cannot remove the replacement + $this->toTheFuture(31 * 60); + $batch = $this->lockService()->getExpiredLocks(1000); + self::assertCount(1, $batch); + $fresh = $this->lockManager->lock(new LockContext($file, ILock::TYPE_USER, self::USER1)); + $this->lockService()->removeLocks($batch); + self::assertSame(1, $this->lockRowCount($file->getId())); + self::assertSame($fresh->getId(), $this->storedLock($file->getId())?->getId()); + } + + /** + * The cleanup reads a batch of expired locks and deletes it afterwards. A + * request whose clock was read before the expiry can refresh one of them in + * between, which reuses the same row; deleting that row by id would drop a + * lock that is valid again. + */ + public function testLockRefreshedAfterTheCleanupBatchWasReadSurvives(): void { + $this->setLockTimeoutMinutes(30); + $this->toTheFuture(0); + $t0 = $this->time; + $folder = $this->loginAndGetUserFolder(self::USER1); + $refreshedFile = $folder->newFile('cron-race.txt', 'AAA'); + $staleFile = $folder->newFile('cron-stale.txt', 'AAA'); + $lock = $this->lockManager->lock(new LockContext($refreshedFile, ILock::TYPE_USER, self::USER1)); + $this->lockManager->lock(new LockContext($staleFile, ILock::TYPE_USER, self::USER1)); + + // the background job wakes up after the expiry and reads its batch + $this->atTime($t0 + 1801); + $batch = $this->lockService()->getExpiredLocks(1000); + self::assertCount(2, $batch); + + // a request that read its clock a moment before the expiry refreshes one + // of them, updating the very row the job is about to delete + $this->atTime($t0 + 1799); + $refreshed = $this->lockManager->lock(new LockContext($refreshedFile, ILock::TYPE_USER, self::USER1)); + self::assertSame($lock->getId(), $refreshed->getId(), 'the refresh updated the same row'); + self::assertSame($t0 + 1799 + 1800, $refreshed->getExpiresAt()); + + // the job now deletes the batch it read earlier + $this->atTime($t0 + 1802); + $this->lockService()->removeLocksIfExpired($batch); + + self::assertSame(1, $this->lockRowCount($refreshedFile->getId()), 'the refreshed lock survives its own cleanup batch'); + self::assertSame(0, $this->lockRowCount($staleFile->getId()), 'a genuinely expired lock is still removed'); + } + + /** + * expires_at is the last second the lock is invalid, not the last second it + * is valid, and every layer has to agree on that. + */ + public function testTheExpirySecondItselfCountsAsExpired(): void { + $this->setLockTimeoutMinutes(10); + $this->toTheFuture(0); + $t0 = $this->time; + $folder = $this->loginAndGetUserFolder(self::USER1); + $file = $folder->newFile('boundary.txt', 'AAA'); + $other = $folder->newFile('boundary-2.txt', 'AAA'); + $this->lockManager->lock(new LockContext($file, ILock::TYPE_USER, self::USER1)); + $this->lockManager->lock(new LockContext($other, ILock::TYPE_USER, self::USER1)); + + // one second earlier nothing is expired anywhere + $this->atTime($t0 + 599); + self::assertSame([], $this->lockService()->getExpiredLocks(), 'not expired one second early'); + self::assertCount(1, $this->lockManager->getLocks($file->getId())); + + // on the second itself the query, the targeted delete and the model agree + $this->atTime($t0 + 600); + self::assertCount(2, $this->lockService()->getExpiredLocks(), 'the cleanup query includes the boundary second'); + self::assertTrue( + \OCP\Server::get(LocksRequest::class)->removeExpired($other->getId(), $t0 + 600), + 'the targeted delete includes the boundary second' + ); + self::assertSame([], $this->lockManager->getLocks($file->getId()), 'the model treats the boundary second as expired'); + self::assertSame(0, $this->lockRowCount($file->getId()), 'and reading it away removed the row'); + } + + public function testAcquireWithExplicitTimeout(): void { + $this->setLockTimeoutMinutes(-1); + $this->toTheFuture(0); + $file = $this->loginAndGetUserFolder(self::USER1)->newFile('explicit.txt', 'AAA'); + $lock = $this->lockService()->acquire(new LockContext($file, ILock::TYPE_TOKEN, self::USER1), 600, 'native-token'); + self::assertSame($this->time + 600, $lock->getExpiresAt()); + self::assertSame('native-token', $lock->getToken()); + + $refreshed = $this->lockService()->acquire(new LockContext($file, ILock::TYPE_TOKEN, 'native-token'), FileLock::ETA_INFINITE); + self::assertSame($lock->getId(), $refreshed->getId()); + self::assertNull($refreshed->getExpiresAt()); + } +} diff --git a/tests/Feature/LockFeatureTest.php b/tests/Feature/LockFeatureTest.php index 75a5047f..f4dcf96d 100644 --- a/tests/Feature/LockFeatureTest.php +++ b/tests/Feature/LockFeatureTest.php @@ -26,7 +26,6 @@ use OCP\Share\IShare; use PHPUnit\Framework\Attributes\Group; use PHPUnit\Framework\MockObject\MockObject; -use Sabre\DAV\Locks\LockInfo; use Sabre\DAV\PropFind; use Test\TestCase; use Test\Util\User\Dummy; @@ -352,19 +351,6 @@ public function testInfiniteLockReportsNoExpiryOverWebdav(): void { self::assertSame(30 * 60, $this->davLockTimeout($expiring)); } - /** - * The standard {DAV:}timeout property has its own sentinel for a lock that - * never expires (RFC4918 renders it as "Infinite", which Sabre only emits - * for a timeout of exactly -1). Sending the raw negative internal lifetime - * there produced the invalid "Second--60"; -60 and 0 are what - * LockService::lock() actually stores for the two "never expires" configs. - */ - public function testInfiniteLockReportsStandardWebdavTimeoutAsInfinite(): void { - self::assertSame(LockInfo::TIMEOUT_INFINITE, (new FileLock(-60))->toLockInfo()->timeout); - self::assertSame(LockInfo::TIMEOUT_INFINITE, (new FileLock(0))->toLockInfo()->timeout); - self::assertSame(30 * 60, (new FileLock(30 * 60))->toLockInfo()->timeout); - } - public function testLockApp(): void { $file = $this->loginAndGetUserFolder(self::TEST_USER1) ->newFile('test-file2', 'AAA'); @@ -618,8 +604,13 @@ private function toTheFuture(int $seconds): void { } private function deleteTestFiles(\OCP\Files\Folder $folder): void { + $lockService = \OCP\Server::get(LockService::class); + $lockService->removeLocks(\OCP\Server::get(\OCA\FilesLock\Db\LocksRequest::class)->getAll()); foreach (self::TEST_FILES as $filename) { - $folder->delete($filename); + try { + $folder->get($filename)->delete(); + } catch (\OCP\Files\NotFoundException) { + } } } diff --git a/tests/Feature/MigrationTest.php b/tests/Feature/MigrationTest.php new file mode 100644 index 00000000..aadde4b1 --- /dev/null +++ b/tests/Feature/MigrationTest.php @@ -0,0 +1,132 @@ +connection = \OCP\Server::get(IDBConnection::class); + } + + protected function tearDown(): void { + $this->runMigration(); + parent::tearDown(); + } + + private function schemaWrapper(): SchemaWrapper { + /** @var ConnectionAdapter $adapter */ + $adapter = $this->connection; + return new SchemaWrapper($adapter->getInner()); + } + + private function runMigration(): void { + $output = $this->createMock(IOutput::class); + $schemaClosure = fn (): SchemaWrapper => $this->schemaWrapper(); + $step1 = new Version36000Date20260906120000($this->connection); + $step1->preSchemaChange($output, $schemaClosure, []); + $schema = $step1->changeSchema($output, $schemaClosure, []); + if ($schema !== null) { + $this->connection->migrateToSchema($schema->getWrappedSchema()); + } + $step1->postSchemaChange($output, $schemaClosure, []); + $step2 = new Version36000Date20260906120100(); + $schema = $step2->changeSchema($output, $schemaClosure, []); + if ($schema !== null) { + $this->connection->migrateToSchema($schema->getWrappedSchema()); + } + } + + /** + * Bring the table back to the shape of the previous release. + */ + private function downgradeSchema(): void { + $schema = $this->schemaWrapper(); + $table = $schema->getTable(LocksRequest::TABLE_LOCKS); + foreach ([Version36000Date20260906120000::INDEX_FILE_ID, Version36000Date20260906120000::INDEX_EXPIRES] as $index) { + if ($table->hasIndex($index)) { + $table->dropIndex($index); + } + } + if ($table->hasColumn('expires_at')) { + $table->dropColumn('expires_at'); + } + $table->addIndex(['file_id'], 'files_lock_old_file_id'); + $this->connection->migrateToSchema($schema->getWrappedSchema()); + } + + private function insertLegacyRow(int $fileId, string $user, string $token, int $creation, int $ttl): int { + $qb = $this->connection->getQueryBuilder(); + $qb->insert(LocksRequest::TABLE_LOCKS) + ->setValue('user_id', $qb->createNamedParameter($user)) + ->setValue('file_id', $qb->createNamedParameter($fileId, IQueryBuilder::PARAM_INT)) + ->setValue('token', $qb->createNamedParameter($token)) + ->setValue('creation', $qb->createNamedParameter($creation, IQueryBuilder::PARAM_INT)) + ->setValue('type', $qb->createNamedParameter(0, IQueryBuilder::PARAM_INT)) + ->setValue('ttl', $qb->createNamedParameter($ttl, IQueryBuilder::PARAM_INT)) + ->setValue('owner', $qb->createNamedParameter($user)); + $qb->executeStatement(); + return $qb->getLastInsertId(); + } + + public function testDuplicatesAreReconciledBeforeUniquenessIsEnforced(): void { + $this->downgradeSchema(); + $table = $this->schemaWrapper()->getTable(LocksRequest::TABLE_LOCKS); + self::assertFalse($table->hasColumn('expires_at')); + self::assertFalse($table->hasIndex(Version36000Date20260906120000::INDEX_FILE_ID)); + + $creation = 1700000000; + $this->insertLegacyRow(4242, 'alice', 'files_lock/a', $creation, 1800); + $this->insertLegacyRow(4242, 'bob', 'files_lock/b', $creation + 10, 1800); + $keep = $this->insertLegacyRow(4242, 'carol', 'files_lock/c', $creation + 20, 0); + $finite = $this->insertLegacyRow(4243, 'dave', 'files_lock/d', $creation, 600); + $infinite = $this->insertLegacyRow(4244, 'erin', 'files_lock/e', $creation, -60); + self::assertSame(3, $this->lockRowCount(4242)); + + $this->runMigration(); + + self::assertSame(1, $this->lockRowCount(4242)); + $request = \OCP\Server::get(LocksRequest::class); + $survivor = $request->getFromFileId(4242); + self::assertSame($keep, $survivor->getId(), 'the most recently created row wins'); + self::assertNull($survivor->getExpiresAt(), 'ttl 0 stays infinite'); + self::assertSame($creation + 600, $request->getFromFileId(4243)->getExpiresAt(), 'creation + ttl becomes the absolute expiry'); + self::assertNull($request->getFromFileId(4244)->getExpiresAt(), 'negative ttl stays infinite'); + self::assertSame($finite, $request->getFromFileId(4243)->getId()); + self::assertSame($infinite, $request->getFromFileId(4244)->getId()); + + $table = $this->schemaWrapper()->getTable(LocksRequest::TABLE_LOCKS); + self::assertTrue($table->hasColumn('expires_at')); + self::assertTrue($table->hasIndex(Version36000Date20260906120000::INDEX_FILE_ID)); + self::assertTrue($table->getIndex(Version36000Date20260906120000::INDEX_FILE_ID)->isUnique()); + self::assertTrue($table->hasIndex(Version36000Date20260906120000::INDEX_EXPIRES)); + self::assertFalse($table->hasIndex('files_lock_old_file_id'), 'the old non-unique index is gone'); + + // running it again is a no-op + $this->runMigration(); + self::assertSame(1, $this->lockRowCount(4242)); + } +} diff --git a/tests/Feature/fixtures/concurrent-lock.php b/tests/Feature/fixtures/concurrent-lock.php new file mode 100644 index 00000000..0d92ed82 --- /dev/null +++ b/tests/Feature/fixtures/concurrent-lock.php @@ -0,0 +1,50 @@ + + * Prints LOCKED, CONFLICT or ERROR
after racing ILockManager::lock(). + */ + +use OCP\Files\IRootFolder; +use OCP\Files\Lock\ILock; +use OCP\Files\Lock\ILockManager; +use OCP\Files\Lock\LockContext; +use OCP\Files\Lock\OwnerLockedException; + +define('OC_CONSOLE', 1); +require_once __DIR__ . '/../../../../../lib/base.php'; +while (ob_get_level() > 0) { + ob_end_clean(); +} + +[, $userId, $fileId, $startAt] = $argv; + +try { + \OC_User::setUserId($userId); + \OC_Util::setupFS($userId); + $node = \OCP\Server::get(IRootFolder::class)->getUserFolder($userId)->getFirstNodeById((int)$fileId); + if ($node === null) { + fwrite(STDOUT, "ERROR file not found\n"); + exit(1); + } + $lockManager = \OCP\Server::get(ILockManager::class); + while (microtime(true) < (float)$startAt) { + usleep(100); + } + $lockManager->lock(new LockContext($node, ILock::TYPE_USER, $userId)); + fwrite(STDOUT, "LOCKED\n"); +} catch (OwnerLockedException) { + fwrite(STDOUT, "CONFLICT\n"); +} catch (\Throwable $e) { + fwrite(STDOUT, 'ERROR ' . $e::class . ': ' . $e->getMessage() . "\n"); + exit(1); +} From afc45c0a5ea2fa1160fb6e4cc324d7b84791fb56 Mon Sep 17 00:00:00 2001 From: Git'Fellow <12234510+solracsf@users.noreply.github.com> Date: Sun, 6 Sep 2026 19:36:54 +0200 Subject: [PATCH 2/8] feat(policy): one authorization decision for every access path Who may release a lock was decided again in every caller, and differently: the file-owner override was keyed on a hard-coded list of mount provider classes, so any provider not on it granted the override to whoever happened to be looking; a token lock could only be released by presenting the token, which the server- side API cannot do, so an app could not clear its own user's stale client lock; and possession of the token was the whole credential even though the token is published to everyone who can read the file. LockPolicy now answers three questions for every lock type: who holds a lock, who may write the file, and who may release it. The service asks it instead of deciding for itself. - a user lock belongs to its user, an app lock to the app's lock scope, a token lock to the token together with the principal it was issued for - the file-owner override applies to files on a user's own home storage, stated positively rather than as everything-except-these-classes - the user a lock is recorded for can always release it, on any path, so the override flag the callers had to remember is gone - releasing a token lock also needs permission to write the file: the token is publicly readable (RFC 4918 section 6.5), so section 6.4 asks for the normal permission mechanism rather than the obscurity of the token - a forced release removes the lock by file id, without resolving the file through the owner, who may well have lost access to it by then Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com> --- lib/Service/LockPolicy.php | 99 +++++++++++++ lib/Service/LockService.php | 209 ++++++++++++++++++---------- tests/Feature/LockFeatureTest.php | 9 +- tests/Feature/UnlockPolicyTest.php | 215 +++++++++++++++++++++++++++++ 4 files changed, 455 insertions(+), 77 deletions(-) create mode 100644 lib/Service/LockPolicy.php create mode 100644 tests/Feature/UnlockPolicyTest.php diff --git a/lib/Service/LockPolicy.php b/lib/Service/LockPolicy.php new file mode 100644 index 00000000..94c6f66e --- /dev/null +++ b/lib/Service/LockPolicy.php @@ -0,0 +1,99 @@ +getType() !== $lock->getType()) { + return false; + } + if ($context->getOwner() === $lock->getOwner()) { + return true; + } + return $lock->getType() === ILock::TYPE_TOKEN && $context->getOwner() === $lock->getToken(); + } + + /** + * @param string|null $viewerId user performing the write, null for a sessionless caller + * @param list $presentedTokens lock tokens presented with the request + * @param LockContext|null $scope active ILockManager scope + */ + public function canWrite(FileLock $lock, ?string $viewerId, array $presentedTokens, ?LockContext $scope): bool { + switch ($lock->getType()) { + case ILock::TYPE_USER: + return $viewerId !== null && $viewerId === $lock->getOwner(); + case ILock::TYPE_APP: + return $scope !== null + && $scope->getType() === ILock::TYPE_APP + && $scope->getOwner() === $lock->getOwner(); + case ILock::TYPE_TOKEN: + $tokenPresented = in_array($lock->getToken(), $presentedTokens, true) + || ($scope !== null && $scope->getType() === ILock::TYPE_TOKEN && $scope->getOwner() === $lock->getToken()); + if (!$tokenPresented) { + return false; + } + return $viewerId === null || $viewerId === $lock->getOwner(); + } + return false; + } + + /** + * @param LockContext $context asserted identity of the caller (owner string is a + * user id, an app id, or a lock token) + * @param string|null $token lock token presented separately from the context + * @param bool $isFileOwner whether the caller owns the file (see LockService::isFileOwner) + * @param bool $canModify whether the caller may write the file + */ + public function canUnlock(FileLock $lock, LockContext $context, ?string $token, bool $isFileOwner, bool $force, bool $canModify = true): bool { + if ($force || $isFileOwner) { + return true; + } + // whoever the lock was recorded for releases it, even if their access to + // the file was reduced while they held it + if ($context->getOwner() === $lock->getOwner()) { + return true; + } + if ($lock->getType() === ILock::TYPE_TOKEN) { + // the token is a public credential (RFC 4918 section 6.5), so possession + // alone cannot be the whole authorization: RFC 4918 section 6.4 requires + // privileges to be enforced by the normal mechanisms rather than by the + // obscurity of the token. Someone who may not write the file could never + // have taken the lock, so they may not release it either. + return $canModify && ($token === $lock->getToken() || $context->getOwner() === $lock->getToken()); + } + return false; + } +} diff --git a/lib/Service/LockService.php b/lib/Service/LockService.php index 71a8a604..63c7ed0f 100644 --- a/lib/Service/LockService.php +++ b/lib/Service/LockService.php @@ -45,7 +45,8 @@ class LockService { private array $lockCache = []; /** @var array */ private array $remoteLockCache = []; - private bool $allowUserOverride = false; + /** @var list */ + private array $presentedTokens = []; public function __construct( private readonly IL10N $l10n, @@ -59,6 +60,7 @@ public function __construct( private readonly IRequest $request, private readonly LoggerInterface $logger, private readonly IRootFolder $rootFolder, + private readonly LockPolicy $policy, ) { } @@ -70,9 +72,34 @@ private function now(): int { return Server::get(ITimeFactory::class)->getTime(); } + public function getPolicy(): LockPolicy { + return $this->policy; + } + + /** + * Register a lock token presented by the current request (WebDAV If header). + */ + public function presentToken(string $token): void { + if ($token !== '' && !in_array($token, $this->presentedTokens, true)) { + $this->presentedTokens[] = $token; + } + } + + /** + * @return list + */ + public function getPresentedTokens(): array { + return $this->presentedTokens; + } + + public function resetPresentedTokens(): void { + $this->presentedTokens = []; + } + public function clearCache(): void { $this->lockCache = []; $this->remoteLockCache = []; + $this->presentedTokens = []; } /** @@ -243,8 +270,7 @@ public function acquire(LockContext $lockScope, ?int $timeout = null, ?string $t */ private function refreshOrConflict(FileLock $known, LockContext $lockScope, int $timeout, int $now): FileLock { $this->injectMetadata($known); - if (!($known->getType() === $lockScope->getType() - && ($known->getOwner() === $lockScope->getOwner() || $known->getToken() === $lockScope->getOwner()))) { + if (!$this->policy->isHolder($known, $lockScope)) { $this->lockCache[$known->getFileId()] = $known; throw new OwnerLockedException($known); } @@ -256,18 +282,38 @@ private function refreshOrConflict(FileLock $known, LockContext $lockScope, int return $known; } + public function update(FileLock $lock): void { + $this->locksRequest->update($lock); + $this->lockCache[$lock->getFileId()] = $lock; + } + + public function getAppName(string $appId): ?string { + /** @var array{name: null}|null $appInfo */ + $appInfo = $this->appManager->getAppInfo($appId); + return $appInfo['name'] ?? null; + } + /** + * Release the lock on the node of $lock. + * + * @param string|null $token lock token presented with the request + * * @throws InvalidPathException * @throws LockNotFoundException * @throws NotFoundException * @throws UnauthorizedUnlockException */ - public function unlock(LockContext $lock, bool $force = false): FileLock { + public function unlock(LockContext $lock, bool $force = false, ?string $token = null): FileLock { $this->logger->notice('unlocking file', ['fileLock' => $lock]); $known = $this->getLockFromFileId($lock->getNode()->getId()); - if (!$force) { - $this->canUnlock($lock, $known); + if (!$this->policy->canUnlock($known, $lock, $token, $this->isFileOwner($lock->getNode()), $force, $this->canModify($lock->getNode()))) { + $this->injectMetadata($known); + throw new UnauthorizedUnlockException( + $known->getType() === ILock::TYPE_TOKEN + ? $this->l10n->t('File can only be unlocked by providing a valid owner lock token') + : $this->l10n->t('File can only be unlocked by the owner of the lock') + ); } $this->locksRequest->delete($known); @@ -277,102 +323,119 @@ public function unlock(LockContext $lock, bool $force = false): FileLock { return $known; } + /** + * @deprecated the owner override is part of the policy; kept for callers of older versions + */ public function enableUserOverride(): void { - $this->allowUserOverride = true; } - public function canUnlock(LockContext $request, FileLock $current): void { - $isSameUser = $current->getOwner() === $this->userSession->getUser()?->getUID(); - $isSameToken = $request->getOwner() === $current->getToken(); - $isSameOwner = $request->getOwner() === $current->getOwner(); - $isSameType = $request->getType() === $current->getType(); - - // we need to ignore some filesystem that return current user as file owner - $ignoreFileOwnership = [ - 'OCA\GroupFolders\Mount\MountProvider', - 'OCA\Files_External\Config\ConfigAdapter' - ]; + /** + * @throws UnauthorizedUnlockException when the node cannot be locked by the caller + * @throws NotFileException when the node is not a file + */ + public function canLock(LockContext $request, ?FileLock $current = null): void { + if (($request->getNode()->getPermissions() & Constants::PERMISSION_UPDATE) === 0) { + throw new UnauthorizedUnlockException( + $this->l10n->t('File can only be locked with update permissions.') + ); + } + } - $isFileOwner = $request->getNode()->getOwner()->getUID() === $this->userSession->getUser()?->getUID() - && !in_array($request->getNode()->getMountPoint()->getMountProvider(), $ignoreFileOwnership); + /** + * Whether the current user may write $lock's file. + * + * @param LockContext|null $scope active ILockManager scope of the caller + */ + public function canWrite(FileLock $lock, ?LockContext $scope): bool { + return $this->policy->canWrite($lock, $this->userSession->getUser()?->getUID(), $this->presentedTokens, $scope); + } - // Check the token for token based locks - if ($current->getType() === ILock::TYPE_TOKEN) { - // token holder can unlock - if ($isSameToken) { - return; - } - // file owner or lock owner can unlock - if ($this->allowUserOverride && ($isSameUser || $isFileOwner)) { - return; - } + /** + * Whether the current user may unlock $current through a request carrying $request. + */ + public function canUnlock(LockContext $request, FileLock $current, ?string $token = null): void { + if (!$this->policy->canUnlock($current, $request, $token, $this->isFileOwner($request->getNode()), false, $this->canModify($request->getNode()))) { throw new UnauthorizedUnlockException( - $this->l10n->t('File can only be unlocked by providing a valid owner lock token') + $this->l10n->t('File can only be unlocked by the owner of the lock') ); } + } - // Otherwise, we check if the owner (user id OR app id) for a match - if ($isSameOwner && $isSameType) { - return; + /** + * The file owner override applies only to files stored in a user's own home + * storage (directly or through a share of it). Group folders, external + * storages and other mounts report the current user as owner of every file, + * so they never grant the override. + */ + /** + * Whether the current caller may write the node at all, independently of any lock. + */ + public function canModify(Node $node): bool { + try { + return ($node->getPermissions() & Constants::PERMISSION_UPDATE) !== 0; + } catch (Exception) { + return false; } + } - if ($request->getType() === ILock::TYPE_USER && $isFileOwner) { - return; + public function isFileOwner(Node $node): bool { + $user = $this->userSession->getUser(); + if ($user === null) { + return false; + } + try { + if (!$node->getStorage()->instanceOfStorage(IHomeStorage::class)) { + return false; + } + return $node->getOwner()?->getUID() === $user->getUID(); + } catch (Exception) { + return false; } - - throw new UnauthorizedUnlockException( - $this->l10n->t('File can only be unlocked by the owner of the lock') - ); } /** + * Release the lock on a file. With $force the lock row is removed without + * resolving the file through anyone's file system. + * * @throws InvalidPathException * @throws LockNotFoundException * @throws NotFoundException * @throws UnauthorizedUnlockException */ - public function unlockFile(int $fileId, ?string $userId, bool $force = false, int $lockType = ILock::TYPE_USER): FileLock { - $lock = $this->getLockForNodeId($fileId); - if (!$lock) { - throw new LockNotFoundException(); - } - + public function unlockFile(int $fileId, string $userId, bool $force = false, int $lockType = ILock::TYPE_USER): FileLock { if ($force) { - $userId = in_array($lock->getType(), [ILock::TYPE_USER, ILock::TYPE_TOKEN]) ? $lock->getOwner() : $userId; - $lockType = $lock->getType(); + return $this->forceUnlock($fileId); } $node = $this->fileService->getFileFromId($userId, $fileId); - $lock = new LockContext( - $node, - $lockType, - $userId, - ); - $this->propagateEtag($lock->getNode()); - return $this->unlock($lock, $force); - } - - public function update(FileLock $lock): void { - $this->locksRequest->update($lock); - $this->lockCache[$lock->getFileId()] = $lock; - } - - public function getAppName(string $appId): ?string { - /** @var array{name: null}|null $appInfo */ - $appInfo = $this->appManager->getAppInfo($appId); - return $appInfo['name'] ?? null; + return $this->unlock(new LockContext($node, $lockType, $userId)); } /** - * @throws UnauthorizedUnlockException when the node cannot be locked by the caller - * @throws NotFileException when the node is not a file + * Administrative removal of a lock by file id. + * + * @throws LockNotFoundException */ - public function canLock(LockContext $request, ?FileLock $current = null): void { - if (($request->getNode()->getPermissions() & Constants::PERMISSION_UPDATE) === 0) { - throw new UnauthorizedUnlockException( - $this->l10n->t('File can only be locked with update permissions.') - ); + public function forceUnlock(int $fileId): FileLock { + $known = $this->getLockFromFileId($fileId); + $this->logger->notice('force unlocking file', ['fileLock' => $known]); + $this->locksRequest->delete($known); + $this->lockCache[$fileId] = false; + + $node = null; + try { + if ($known->getType() !== ILock::TYPE_APP && $this->userManager->userExists($known->getOwner())) { + $node = $this->rootFolder->getUserFolder($known->getOwner())->getFirstNodeById($fileId); + } + $node ??= $this->rootFolder->getFirstNodeById($fileId); + } catch (Exception) { + } + if ($node !== null) { + $this->propagateEtag($node); } + + $this->injectMetadata($known); + return $known; } /** diff --git a/tests/Feature/LockFeatureTest.php b/tests/Feature/LockFeatureTest.php index f4dcf96d..706838b3 100644 --- a/tests/Feature/LockFeatureTest.php +++ b/tests/Feature/LockFeatureTest.php @@ -40,7 +40,7 @@ class LockFeatureTest extends TestCase { * * @var list */ - private const TEST_FILES = [ + private const array TEST_FILES = [ 'test-file', 'test-file2', 'test-file3', @@ -516,16 +516,17 @@ public function testUnlockStaleClientLock(): void { $this->assertCount(1, $locks); // Other users cannot unlock + $sharedFile = $this->loginAndGetUserFolder(self::TEST_USER2)->get('test-file-client'); try { - $this->lockManager->unlock(new LockContext($file, ILock::TYPE_TOKEN, self::TEST_USER2)); + $this->lockManager->unlock(new LockContext($sharedFile, ILock::TYPE_TOKEN, self::TEST_USER2)); $locks = []; } catch (\OCP\PreConditionNotMetException) { $locks = $this->lockManager->getLocks($file->getId()); } $this->assertCount(1, $locks); - // The owner can stil force unlock it as done through the OCS controller - \OCP\Server::get(\OCA\FilesLock\Service\LockService::class)->enableUserOverride(); + // The owner can still unlock it, the override is part of the policy on every path + $file = $this->loginAndGetUserFolder(self::TEST_USER1)->get('test-file-client'); $this->lockManager->unlock(new LockContext($file, ILock::TYPE_USER, self::TEST_USER1)); $locks = $this->lockManager->getLocks($file->getId()); diff --git a/tests/Feature/UnlockPolicyTest.php b/tests/Feature/UnlockPolicyTest.php new file mode 100644 index 00000000..1a29b986 --- /dev/null +++ b/tests/Feature/UnlockPolicyTest.php @@ -0,0 +1,215 @@ +lockService()->clearCache(); + } + + private function assertRefused(callable $unlock, int $fileId): void { + try { + $unlock(); + self::fail('unlock should have been refused'); + } catch (UnauthorizedUnlockException|PreConditionNotMetException) { + } + self::assertSame(1, $this->lockRowCount($fileId)); + } + + /** + * @return array{File, File, File} as USER1 (file owner), USER2 (writer), USER3 (writer) + */ + private function sharedViews(string $name): array { + $owner = $this->sharedFile($name, 19, 19); + \OC_Util::setupFS(self::USER2); + \OC_Util::setupFS(self::USER3); + return [ + $owner, + $this->rootFolder->getUserFolder(self::USER2)->get($name), + $this->rootFolder->getUserFolder(self::USER3)->get($name), + ]; + } + + public function testUserLockRelease(): void { + [$owner, $creator, $other] = $this->sharedViews('user.txt'); + $this->actAs(self::USER2); + $this->lockManager->lock(new LockContext($creator, ILock::TYPE_USER, self::USER2)); + $id = $owner->getId(); + + $this->actAs(self::USER3); + $this->assertRefused(fn () => $this->lockManager->unlock(new LockContext($other, ILock::TYPE_USER, self::USER3)), $id); + + $this->actAs(self::USER2); + $this->lockManager->unlock(new LockContext($creator, ILock::TYPE_USER, self::USER2)); + self::assertSame(0, $this->lockRowCount($id)); + + $this->lockManager->lock(new LockContext($creator, ILock::TYPE_USER, self::USER2)); + $this->actAs(self::USER1); + $this->lockManager->unlock(new LockContext($owner, ILock::TYPE_USER, self::USER1)); + self::assertSame(0, $this->lockRowCount($id), 'the file owner overrides a user lock on a file of their home storage'); + } + + public function testTokenLockRelease(): void { + [$owner, $creator, $other] = $this->sharedViews('token.txt'); + $this->actAs(self::USER2); + $lock = $this->lockService()->acquire(new LockContext($creator, ILock::TYPE_TOKEN, self::USER2), null, self::TOKEN); + $id = $owner->getId(); + + $this->actAs(self::USER3); + $this->assertRefused(fn () => $this->lockManager->unlock(new LockContext($other, ILock::TYPE_TOKEN, self::USER3)), $id); + + // the recorded owner releases through the public API without the token (N-01) + $this->actAs(self::USER2); + $this->lockManager->unlock(new LockContext($creator, ILock::TYPE_TOKEN, self::USER2)); + self::assertSame(0, $this->lockRowCount($id)); + + // whoever presents the token releases (RFC 4918 section 6.5 semantics of the native path) + $this->lockService()->acquire(new LockContext($creator, ILock::TYPE_TOKEN, self::USER2), null, self::TOKEN); + $this->actAs(self::USER3); + $this->lockManager->unlock(new LockContext($other, ILock::TYPE_TOKEN, $lock->getToken())); + self::assertSame(0, $this->lockRowCount($id)); + + $this->actAs(self::USER2); + $this->lockService()->acquire(new LockContext($creator, ILock::TYPE_TOKEN, self::USER2), null, self::TOKEN); + $this->actAs(self::USER3); + $this->lockService()->unlock(new LockContext($other, ILock::TYPE_TOKEN, self::USER3), false, $lock->getToken()); + self::assertSame(0, $this->lockRowCount($id)); + + // the file owner overrides a stale client lock + $this->actAs(self::USER2); + $this->lockService()->acquire(new LockContext($creator, ILock::TYPE_TOKEN, self::USER2), null, self::TOKEN); + $this->actAs(self::USER1); + $this->lockManager->unlock(new LockContext($owner, ILock::TYPE_USER, self::USER1)); + self::assertSame(0, $this->lockRowCount($id)); + } + + /** + * The token is publicly readable, so it cannot be the whole authorization: + * a user who may not write the file could never have taken the lock and may + * not release it either. + */ + public function testTokenLockNeedsWritePermissionToRelease(): void { + $owner = $this->sharedFile('token-readonly.txt', 19, 1); + \OC_Util::setupFS(self::USER2); + \OC_Util::setupFS(self::USER3); + $id = $owner->getId(); + + $this->actAs(self::USER2); + $writerView = $this->rootFolder->getUserFolder(self::USER2)->get('token-readonly.txt'); + $lock = $this->lockService()->acquire(new LockContext($writerView, ILock::TYPE_TOKEN, self::USER2), null, self::TOKEN); + + // the read-only recipient can see the token but may not act on it + $this->actAs(self::USER3); + $readerView = $this->rootFolder->getUserFolder(self::USER3)->get('token-readonly.txt'); + $this->assertRefused( + fn (): FileLock => $this->lockService()->unlock(new LockContext($readerView, ILock::TYPE_TOKEN, self::USER3), false, $lock->getToken()), + $id + ); + + // someone who may write the file and presents the token still releases it + $this->actAs(self::USER2); + $this->lockService()->unlock(new LockContext($writerView, ILock::TYPE_TOKEN, self::USER3), false, $lock->getToken()); + self::assertSame(0, $this->lockRowCount($id)); + } + + public function testAppLockRelease(): void { + [$owner, $writer] = $this->sharedViews('app.txt'); + $this->lockManager->lock(new LockContext($owner, ILock::TYPE_APP, 'text')); + $id = $owner->getId(); + + $this->actAs(self::USER2); + $this->assertRefused(fn () => $this->lockManager->unlock(new LockContext($writer, ILock::TYPE_USER, self::USER2)), $id); + $this->assertRefused(fn () => $this->lockManager->unlock(new LockContext($writer, ILock::TYPE_APP, 'other')), $id); + + $this->lockManager->unlock(new LockContext($writer, ILock::TYPE_APP, 'text')); + self::assertSame(0, $this->lockRowCount($id)); + + $this->lockManager->lock(new LockContext($owner, ILock::TYPE_APP, 'text')); + $this->actAs(self::USER1); + $this->lockManager->unlock(new LockContext($owner, ILock::TYPE_USER, self::USER1)); + self::assertSame(0, $this->lockRowCount($id), 'the file owner overrides an app lock'); + } + + public function testOwnerOverrideIsLimitedToHomeStorage(): void { + $storage = new OwnerIsViewerStorage(['storage' => new Temporary([])]); + $this->loginAndGetUserFolder(self::USER1); + \OC_Util::setupFS(self::USER2); + Filesystem::mount($storage, [], '/' . self::USER1 . '/files/shared-mount/'); + Filesystem::mount($storage, [], '/' . self::USER2 . '/files/shared-mount/'); + $file = $this->rootFolder->getUserFolder(self::USER2)->get('shared-mount')->newFile('doc.txt', 'AAA'); + + $this->actAs(self::USER2); + $this->lockManager->lock(new LockContext($file, ILock::TYPE_USER, self::USER2)); + + $this->actAs(self::USER1); + $asUser1 = $this->rootFolder->getUserFolder(self::USER1)->get('shared-mount/doc.txt'); + self::assertSame(self::USER1, $asUser1->getOwner()?->getUID(), 'the mount reports the viewer as owner'); + $this->assertRefused(fn () => $this->lockManager->unlock(new LockContext($asUser1, ILock::TYPE_USER, self::USER1)), $file->getId()); + + $this->lockService()->forceUnlock($file->getId()); + self::assertSame(0, $this->lockRowCount($file->getId())); + } + + public function testForcedUnlockDoesNotNeedOwnerAccess(): void { + [$owner, $creator] = $this->sharedViews('force.txt'); + $this->actAs(self::USER2); + $this->lockManager->lock(new LockContext($creator, ILock::TYPE_USER, self::USER2)); + + $shareManager = \OCP\Server::get(IShareManager::class); + foreach ($shareManager->getSharesBy(self::USER1, \OCP\Share\IShare::TYPE_USER, $owner) as $share) { + $shareManager->deleteShare($share); + } + $this->actAs(''); + + $removed = $this->lockService()->unlockFile($owner->getId(), '', true); + self::assertSame(self::USER2, $removed->getOwner()); + self::assertSame(0, $this->lockRowCount($owner->getId())); + + $this->expectException(LockNotFoundException::class); + $this->lockService()->forceUnlock($owner->getId()); + } + + public function testUnlockWithoutLockReportsNotFound(): void { + $file = $this->loginAndGetUserFolder(self::USER1)->newFile('none.txt', 'AAA'); + $this->expectException(PreConditionNotMetException::class); + $this->lockManager->unlock(new LockContext($file, ILock::TYPE_USER, self::USER1)); + } +} From fb96f00b5c67c2c3cd540a2a30f36168d22752e5 Mon Sep 17 00:00:00 2001 From: Git'Fellow <12234510+solracsf@users.noreply.github.com> Date: Sun, 6 Sep 2026 17:36:01 +0200 Subject: [PATCH 3/8] feat(storage): enforce locks by file identity, including ancestors and deletion The storage wrapper resolved the file by rebuilding a path inside the user's home folder, so it silently enforced nothing on any storage whose internal paths do not start with files/: group and team folders, external storages, every custom mount. It also asked its own question about who may write, which is the policy's job, and it never looked at what a folder contains, so deleting or moving a parent took a locked file with it. - the wrapper resolves the file through the wrapped storage's own cache and asks the policy whether the caller may write it, which makes it work the same on every mount and for every lock type - deleting or moving a directory is refused while it holds a file locked by someone else, found with one query against the file cache rather than by walking the tree - the wrapper runs as the outermost one, ahead of the trash bin, so a refused delete cannot have moved the file to the trash first - a lock is dropped when its file goes away, both from the storage that removed it and from the node and cache events, so a restored file comes back unlocked Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com> --- lib/AppInfo/Application.php | 11 ++ lib/Db/LocksRequest.php | 43 ++++ .../BeforeFileSystemSetupListener.php | 8 +- lib/Listeners/NodeDeletedListener.php | 45 +++++ lib/Service/LockService.php | 48 +++++ lib/Storage/LockWrapper.php | 162 +++++++-------- tests/Feature/AncestorProtectionTest.php | 152 ++++++++++++++ tests/Feature/LifecycleTest.php | 125 ++++++++++++ tests/Feature/WritePolicyTest.php | 186 ++++++++++++++++++ 9 files changed, 697 insertions(+), 83 deletions(-) create mode 100644 lib/Listeners/NodeDeletedListener.php create mode 100644 tests/Feature/AncestorProtectionTest.php create mode 100644 tests/Feature/LifecycleTest.php create mode 100644 tests/Feature/WritePolicyTest.php diff --git a/lib/AppInfo/Application.php b/lib/AppInfo/Application.php index c0ea6f91..f7e79969 100644 --- a/lib/AppInfo/Application.php +++ b/lib/AppInfo/Application.php @@ -14,14 +14,17 @@ use OCA\FilesLock\ConfigLexicon; use OCA\FilesLock\Listeners\BeforeFileSystemSetupListener; use OCA\FilesLock\Listeners\LoadAdditionalScripts; +use OCA\FilesLock\Listeners\NodeDeletedListener; use OCA\FilesLock\Listeners\PropfindPropertiesListener; use OCA\FilesLock\LockProvider; use OCP\AppFramework\App; use OCP\AppFramework\Bootstrap\IBootContext; use OCP\AppFramework\Bootstrap\IBootstrap; use OCP\AppFramework\Bootstrap\IRegistrationContext; +use OCP\Files\Cache\CacheEntryRemovedEvent; use OCP\Files\Events\BeforeFileSystemSetupEvent; use OCP\Files\Events\BeforeRemotePropfindEvent; +use OCP\Files\Events\Node\NodeDeletedEvent; use OCP\Files\Lock\ILockManager; class Application extends App implements IBootstrap { @@ -55,6 +58,14 @@ public function register(IRegistrationContext $context): void { BeforeFileSystemSetupEvent::class, BeforeFileSystemSetupListener::class ); + $context->registerEventListener( + NodeDeletedEvent::class, + NodeDeletedListener::class + ); + $context->registerEventListener( + CacheEntryRemovedEvent::class, + NodeDeletedListener::class + ); $context->registerConfigLexicon(ConfigLexicon::class); } diff --git a/lib/Db/LocksRequest.php b/lib/Db/LocksRequest.php index d67b15f7..8c35e43b 100644 --- a/lib/Db/LocksRequest.php +++ b/lib/Db/LocksRequest.php @@ -208,6 +208,49 @@ public function getExpired(int $now, int $limit = 0): array { return $this->getLocksFromRequest($qb->executeQuery()); } + /** + * Locks on files stored below a folder, resolved through the file cache so it + * works for every storage type. Each entry is the lock plus the path of the + * locked file relative to the folder. + * + * @return list + */ + public function getLocksBelow(int $folderId): array { + $qb = $this->connection->getQueryBuilder(); + $qb->select('storage', 'path') + ->from('filecache') + ->where($qb->expr()->eq('fileid', $qb->createNamedParameter($folderId, IQueryBuilder::PARAM_INT))); + $result = $qb->executeQuery(); + $folder = $result->fetch(); + $result->closeCursor(); + if ($folder === false) { + return []; + } + + $prefix = ($folder['path'] === null || $folder['path'] === '') ? '' : $folder['path'] . '/'; + + $qb = $this->connection->getQueryBuilder(); + $qb->select('l.id', 'l.user_id', 'l.file_id', 'l.token', 'l.creation', 'l.type', 'l.ttl', 'l.owner', 'l.scope', 'l.expires_at', 'f.path') + ->from(self::TABLE_LOCKS, 'l') + ->innerJoin('l', 'filecache', 'f', $qb->expr()->eq('l.file_id', 'f.fileid')) + ->where($qb->expr()->eq('f.storage', $qb->createNamedParameter((int)$folder['storage'], IQueryBuilder::PARAM_INT))); + if ($prefix !== '') { + $qb->andWhere($qb->expr()->like('f.path', $qb->createNamedParameter($this->connection->escapeLikeParameter($prefix) . '%'))); + } + + $locks = []; + $result = $qb->executeQuery(); + while ($row = $result->fetch()) { + $locks[] = [ + 'lock' => $this->parseLockSelectSql($row), + 'path' => substr((string)$row['path'], strlen($prefix)), + ]; + } + $result->closeCursor(); + + return $locks; + } + /** * @throws LockNotFoundException */ diff --git a/lib/Listeners/BeforeFileSystemSetupListener.php b/lib/Listeners/BeforeFileSystemSetupListener.php index 8a31b43a..a0a577ec 100644 --- a/lib/Listeners/BeforeFileSystemSetupListener.php +++ b/lib/Listeners/BeforeFileSystemSetupListener.php @@ -10,7 +10,6 @@ namespace OCA\FilesLock\Listeners; -use OCA\FilesLock\Service\FileService; use OCA\FilesLock\Service\LockService; use OCA\FilesLock\Storage\LockWrapper; use OCP\EventDispatcher\Event; @@ -28,7 +27,6 @@ class BeforeFileSystemSetupListener implements IEventListener { public function __construct( private readonly ILockManager $lockManager, private readonly IUserSession $userSession, - private readonly FileService $fileService, private readonly LockService $lockService, ) { } @@ -39,15 +37,17 @@ public function handle(Event $event): void { return; } + // priority 0 makes this the outermost wrapper (lower numbers are applied + // last), ahead of the trash bin (1) and encryption (2): a locked file must + // be refused before anything moves it to the trash or rewrites it $event->addStorageWrapper( LockWrapper::class, fn (string $mountPoint, IStorage $storage): LockWrapper => new LockWrapper( [ 'storage' => $storage, 'lock_manager' => $this->lockManager, 'user_session' => $this->userSession, - 'file_service' => $this->fileService, 'lock_service' => $this->lockService, ] - ), 10); + ), 0); } } diff --git a/lib/Listeners/NodeDeletedListener.php b/lib/Listeners/NodeDeletedListener.php new file mode 100644 index 00000000..45e1aa1c --- /dev/null +++ b/lib/Listeners/NodeDeletedListener.php @@ -0,0 +1,45 @@ + + */ +class NodeDeletedListener implements IEventListener { + public function __construct( + private readonly LockService $lockService, + ) { + } + + #[\Override] + public function handle(Event $event): void { + if ($event instanceof NodeDeletedEvent) { + $fileId = $event->getNode()->getId(); + } elseif ($event instanceof CacheEntryRemovedEvent) { + $fileId = $event->getFileId(); + } else { + return; + } + + if ($fileId !== null && $fileId > 0) { + $this->lockService->removeLocksForFileIds([(int)$fileId]); + } + } +} diff --git a/lib/Service/LockService.php b/lib/Service/LockService.php index 63c7ed0f..f1f10d70 100644 --- a/lib/Service/LockService.php +++ b/lib/Service/LockService.php @@ -438,6 +438,21 @@ public function forceUnlock(int $fileId): FileLock { return $known; } + /** + * Remove every lock of the given files, regardless of ownership. + * + * @param list $fileIds + */ + public function removeLocksForFileIds(array $fileIds): void { + if (empty($fileIds)) { + return; + } + $this->locksRequest->removeByFileIds($fileIds); + foreach ($fileIds as $fileId) { + $this->lockCache[$fileId] = false; + } + } + /** * Locks whose expiry has passed. * @@ -484,6 +499,39 @@ public function getLockFromFileId(int $fileId): FileLock { return $lock; } + /** + * Locks on files below a folder that the current user may not write. + * + * @param LockContext|null $scope active ILockManager scope of the caller + * @return list + */ + public function getBlockingLocksBelow(int $folderId, ?LockContext $scope): array { + $now = $this->now(); + $blocking = []; + foreach ($this->locksRequest->getLocksBelow($folderId) as $entry) { + if ($entry['lock']->isExpired($now)) { + continue; + } + if (!$this->canWrite($entry['lock'], $scope)) { + $blocking[] = $entry; + } + } + return $blocking; + } + + /** + * Active locks on files below a folder (relative path included). + * + * @return list + */ + public function getLocksBelow(int $folderId): array { + $now = $this->now(); + return array_values(array_filter( + $this->locksRequest->getLocksBelow($folderId), + fn (array $entry): bool => !$entry['lock']->isExpired($now) + )); + } + public function injectMetadata(FileLock $lock): FileLock { $displayName = null; if ($lock->getType() === ILock::TYPE_USER) { diff --git a/lib/Storage/LockWrapper.php b/lib/Storage/LockWrapper.php index ba9dc283..b7db3c2a 100644 --- a/lib/Storage/LockWrapper.php +++ b/lib/Storage/LockWrapper.php @@ -8,33 +8,25 @@ namespace OCA\FilesLock\Storage; use OC\Files\Storage\Wrapper\Wrapper; -use OCA\FilesLock\Exceptions\LockNotFoundException; use OCA\FilesLock\Model\FileLock; -use OCA\FilesLock\Service\FileService; use OCA\FilesLock\Service\LockService; use OCP\Constants; -use OCP\Files\InvalidPathException; -use OCP\Files\Lock\ILock; use OCP\Files\Lock\ILockManager; -use OCP\Files\Lock\NoLockProviderException; -use OCP\Files\NotFoundException; use OCP\Files\Storage\IStorage; -use OCP\IUserSession; use OCP\Lock\LockedException; use OCP\Lock\ManuallyLockedException; +/** + * Enforces file locks for every write that reaches a storage, whatever mount the + * storage is attached to. Files are identified through the wrapped storage's own + * cache, so the check does not depend on the shape of the storage path. + */ class LockWrapper extends Wrapper { private readonly ILockManager $lockManager; - /** @var FileService */ - private $fileService; - /** @var LockService */ private $lockService; - /** @var IUserSession */ - private $userSession; - /** * LockWrapper constructor. * @@ -44,8 +36,6 @@ public function __construct(array $arguments) { parent::__construct($arguments); $this->lockManager = $arguments['lock_manager']; - $this->userSession = $arguments['user_session']; - $this->fileService = $arguments['file_service']; $this->lockService = $arguments['lock_service']; } @@ -56,65 +46,49 @@ public function __construct(array $arguments) { * @throws LockedException */ protected function checkPermissions($path, $permissions): bool { - $viewerId = ''; - $user = $this->userSession->getUser(); - if ($user !== null) { - $viewerId = $user->getUID(); - $ownerId = $viewerId; - } else { - $ownerId = $this->getOwner($path); + if ($permissions === Constants::PERMISSION_READ) { + return true; } - /** @var FileLock $lock */ - if (!$this->isPathLocked($ownerId, $path, $viewerId, $lock)) { + $fileId = $this->getCache()->getId($path); + if ($fileId === -1) { return true; } - switch ($permissions) { - case Constants::PERMISSION_READ: - return true; - case Constants::PERMISSION_DELETE: - case Constants::PERMISSION_UPDATE: - throw new ManuallyLockedException( - $path, null, $lock->getToken(), $lock->getOwner(), $lock->getETA() - ); - - default: - return false; + $lock = $this->lockService->getActiveLock($fileId); + if ($lock === null || $this->lockService->canWrite($lock, $this->lockManager->getLockInScope())) { + return true; } + + throw new ManuallyLockedException( + $path, null, $lock->getToken(), $lock->getOwner(), $lock->getETA() + ); } - protected function isPathLocked(string $ownerId, string $path, string $viewerId, ?FileLock &$lock = null): bool { - try { - $file = $this->fileService->getFileFromPath($ownerId, $path); - } catch (NotFoundException) { - return false; + /** + * Refuse an operation on a directory that would delete or relocate a locked + * descendant the current user may not write. + * + * @throws LockedException + */ + protected function checkDescendants(IStorage $storage, string $path): void { + if (!$storage->is_dir($path)) { + return; } - - if ($file->getId() === null) { - return false; + $folderId = $storage->getCache()->getId($path); + if ($folderId === -1) { + return; } - return $this->isFileLocked($file->getId(), $viewerId, $lock); - } - - protected function isFileLocked(int $fileId, string $viewerId, ?FileLock &$lock = null): bool { - try { - $lock = $this->lockService->getLockFromFileId($fileId); - if ($lock->getType() === ILock::TYPE_USER && $lock->getOwner() !== $viewerId) { - return true; - } - - if ($lock->getType() === ILock::TYPE_APP) { - $lockScope = $this->lockManager->getLockInScope(); - if (!$lockScope || $lockScope->getType() !== $lock->getType() || $lockScope->getOwner() !== $lock->getOwner()) { - return true; - } - } - } catch (NoLockProviderException|LockNotFoundException|InvalidPathException|NotFoundException) { + $blocking = $this->lockService->getBlockingLocksBelow($folderId, $this->lockManager->getLockInScope()); + if ($blocking === []) { + return; } - - return false; + /** @var FileLock $lock */ + $lock = $blocking[0]['lock']; + throw new ManuallyLockedException( + rtrim($path, '/') . '/' . $blocking[0]['path'], null, $lock->getToken(), $lock->getOwner(), $lock->getETA() + ); } #[\Override] @@ -123,19 +97,16 @@ public function rename(string $source, string $target): bool { $part = substr($source, strlen($target)); //This is a rename of the transfer file to the original file if (str_starts_with($part, '.ocTransferId')) { - return $this->checkPermissions($target, Constants::PERMISSION_CREATE) + return $this->checkPermissions($target, Constants::PERMISSION_UPDATE) && parent::rename($source, $target); } } $permissions = $this->file_exists($target) ? Constants::PERMISSION_UPDATE : Constants::PERMISSION_CREATE; - $sourceParent = dirname($source); - if ($sourceParent === '.') { - $sourceParent = ''; - } - return $this->checkPermissions($sourceParent, Constants::PERMISSION_DELETE) - && $this->checkPermissions($source, Constants::PERMISSION_UPDATE & Constants::PERMISSION_READ) + $this->checkDescendants($this, $source); + + return $this->checkPermissions($source, Constants::PERMISSION_UPDATE) && $this->checkPermissions($target, $permissions) && parent::rename($source, $target); } @@ -153,17 +124,33 @@ public function copy(string $source, string $target): bool { #[\Override] public function copyFromStorage(IStorage $sourceStorage, string $sourceInternalPath, string $targetInternalPath): bool { - $cache = $sourceStorage->getCache(); - $fileId = $cache->getId($sourceInternalPath); - - $user = $this->userSession->getUser(); - if ($fileId > 0 && $this->isFileLocked($fileId, $user?->getUID() ?? '', $lock)) { - throw new ManuallyLockedException($sourceInternalPath, null, $lock->getToken(), $lock->getOwner(), $lock->getETA()); + $fileId = $sourceStorage->getCache()->getId($sourceInternalPath); + if ($fileId > 0) { + $lock = $this->lockService->getActiveLock($fileId); + if ($lock !== null && !$this->lockService->canWrite($lock, $this->lockManager->getLockInScope())) { + throw new ManuallyLockedException($sourceInternalPath, null, $lock->getToken(), $lock->getOwner(), $lock->getETA()); + } } return parent::copyFromStorage($sourceStorage, $sourceInternalPath, $targetInternalPath); } + #[\Override] + public function moveFromStorage(IStorage $sourceStorage, string $sourceInternalPath, string $targetInternalPath): bool { + $this->checkDescendants($sourceStorage, $sourceInternalPath); + $fileId = $sourceStorage->getCache()->getId($sourceInternalPath); + if ($fileId > 0) { + $lock = $this->lockService->getActiveLock($fileId); + if ($lock !== null && !$this->lockService->canWrite($lock, $this->lockManager->getLockInScope())) { + throw new ManuallyLockedException($sourceInternalPath, null, $lock->getToken(), $lock->getOwner(), $lock->getETA()); + } + } + $permissions = $this->file_exists($targetInternalPath) ? Constants::PERMISSION_UPDATE : Constants::PERMISSION_CREATE; + + return $this->checkPermissions($targetInternalPath, $permissions) + && parent::moveFromStorage($sourceStorage, $sourceInternalPath, $targetInternalPath); + } + #[\Override] public function touch(string $path, ?int $mtime = null): bool { $permissions @@ -179,14 +166,31 @@ public function mkdir(string $path): bool { #[\Override] public function rmdir(string $path): bool { - return $this->checkPermissions($path, Constants::PERMISSION_DELETE) - && parent::rmdir($path); + $this->checkDescendants($this, $path); + $this->checkPermissions($path, Constants::PERMISSION_DELETE); + + $folderId = $this->getCache()->getId($path); + $lockedIds = $folderId > 0 + ? array_map(fn (array $entry): int => $entry['lock']->getFileId(), $this->lockService->getLocksBelow($folderId)) + : []; + + $result = parent::rmdir($path); + if ($result && $lockedIds !== []) { + $this->lockService->removeLocksForFileIds($lockedIds); + } + return $result; } #[\Override] public function unlink(string $path): bool { - return $this->checkPermissions($path, Constants::PERMISSION_DELETE) - && parent::unlink($path); + $this->checkPermissions($path, Constants::PERMISSION_DELETE); + $fileId = $this->getCache()->getId($path); + + $result = parent::unlink($path); + if ($result && $fileId > 0) { + $this->lockService->removeLocksForFileIds([$fileId]); + } + return $result; } #[\Override] diff --git a/tests/Feature/AncestorProtectionTest.php b/tests/Feature/AncestorProtectionTest.php new file mode 100644 index 00000000..2d7f3f8c --- /dev/null +++ b/tests/Feature/AncestorProtectionTest.php @@ -0,0 +1,152 @@ +lockService()->clearCache(); + } + + /** + * USER1 owns dir/sub/inner/locked.txt (locked by USER1) and shares dir with USER2. + * + * @return array{File, Folder} the locked file (USER1 view) and dir (USER2 view) + */ + private function sharedTree(): array { + $root = $this->loginAndGetUserFolder(self::USER1); + $dir = $root->newFolder('dir'); + $inner = $dir->newFolder('sub')->newFolder('inner'); + $locked = $inner->newFile('locked.txt', 'important'); + $this->shareWith($dir, self::USER1, self::USER2, 31); + $this->lockManager->lock(new LockContext($locked, ILock::TYPE_USER, self::USER1)); + $this->trashBefore = $this->trashEntries(); + + \OC_Util::setupFS(self::USER2); + $this->actAs(self::USER2); + return [$locked, $this->rootFolder->getUserFolder(self::USER2)->get('dir')]; + } + + private function trashEntries(): int { + try { + $trash = $this->rootFolder->getUserFolder(self::USER1)->getParent()->get('files_trashbin/files'); + return count($trash->getDirectoryListing()); + } catch (NotFoundException) { + return 0; + } + } + + private function assertIntact(File $locked): void { + $this->actAs(self::USER1); + $this->lockService()->clearCache(); + $file = $this->rootFolder->getUserFolder(self::USER1)->get('dir/sub/inner/locked.txt'); + self::assertSame($locked->getId(), $file->getId()); + self::assertSame('important', $file->getContent()); + self::assertSame(1, $this->lockRowCount($locked->getId())); + self::assertSame($this->trashBefore, $this->trashEntries(), 'nothing was moved to the trash bin'); + $this->actAs(self::USER2); + $this->lockService()->clearCache(); + } + + private function assertLocked(callable $operation, File $locked): void { + try { + $operation(); + self::fail('operation on an ancestor of a locked file should be refused'); + } catch (LockedException) { + } + $this->assertIntact($locked); + } + + public function testDirectOperationsOnLockedFileAreRefused(): void { + [$locked, $dir] = $this->sharedTree(); + $file = $dir->get('sub/inner/locked.txt'); + $this->assertLocked(fn () => $file->delete(), $locked); + $this->assertLocked(fn () => $file->move($dir->getPath() . '/moved.txt'), $locked); + } + + public function testParentOperationsAreRefused(): void { + [$locked, $dir] = $this->sharedTree(); + $inner = $dir->get('sub/inner'); + $this->assertLocked(fn () => $inner->delete(), $locked); + $this->assertLocked(fn () => $inner->move($dir->getPath() . '/inner-moved'), $locked); + $this->assertLocked(fn () => $inner->move($dir->getPath() . '/sub/renamed'), $locked); + } + + public function testNestedParentOperationsAreRefused(): void { + [$locked, $dir] = $this->sharedTree(); + $sub = $dir->get('sub'); + $this->assertLocked(fn () => $sub->delete(), $locked); + $this->assertLocked(fn () => $sub->move($dir->getPath() . '/sub-moved'), $locked); + } + + public function testStorageLevelOperationsAreRefused(): void { + // equivalent of a deployment without the trash bin: the storage is hit directly + [$locked, $dir] = $this->sharedTree(); + $storage = $dir->getStorage(); + $internal = $dir->getInternalPath(); + $subPath = ($internal === '' ? '' : $internal . '/') . 'sub'; + $this->assertLocked(fn () => $storage->rmdir($subPath), $locked); + $this->assertLocked(fn () => $storage->rename($subPath, $subPath . '-moved'), $locked); + self::assertTrue($storage->file_exists($subPath . '/inner/locked.txt')); + } + + public function testLockOwnerMayOperateOnAncestors(): void { + [$locked] = $this->sharedTree(); + $this->actAs(self::USER1); + $root = $this->rootFolder->getUserFolder(self::USER1); + $root->get('dir/sub')->move($root->getPath() . '/dir/sub-moved'); + self::assertSame('important', $root->get('dir/sub-moved/inner/locked.txt')->getContent()); + self::assertSame(1, $this->lockRowCount($locked->getId()), 'a move keeps the lock'); + $root->get('dir/sub-moved')->delete(); + self::assertSame(0, $this->lockRowCount($locked->getId()), 'deleting the file removes its lock'); + } + + public function testAncestorProtectionOnNonHomeMount(): void { + $storage = new Temporary([]); + $this->loginAndGetUserFolder(self::USER1); + \OC_Util::setupFS(self::USER2); + Filesystem::mount($storage, [], '/' . self::USER1 . '/files/ext/'); + Filesystem::mount($storage, [], '/' . self::USER2 . '/files/ext/'); + $locked = $this->rootFolder->getUserFolder(self::USER1)->get('ext')->newFolder('sub')->newFile('locked.txt', 'important'); + $this->lockManager->lock(new LockContext($locked, ILock::TYPE_USER, self::USER1)); + + $this->actAs(self::USER2); + $sub = $this->rootFolder->getUserFolder(self::USER2)->get('ext/sub'); + try { + $sub->delete(); + self::fail('deleting the parent on a non-home mount must be refused'); + } catch (LockedException) { + } + try { + $sub->move($this->rootFolder->getUserFolder(self::USER2)->getPath() . '/ext/sub2'); + self::fail('moving the parent on a non-home mount must be refused'); + } catch (LockedException) { + } + self::assertSame('important', $this->rootFolder->getUserFolder(self::USER2)->get('ext/sub/locked.txt')->getContent()); + self::assertSame(1, $this->lockRowCount($locked->getId())); + } +} diff --git a/tests/Feature/LifecycleTest.php b/tests/Feature/LifecycleTest.php new file mode 100644 index 00000000..db808737 --- /dev/null +++ b/tests/Feature/LifecycleTest.php @@ -0,0 +1,125 @@ +loginAndGetUserFolder(self::USER1)->newFile('deleted.txt', 'AAA'); + $id = $file->getId(); + $this->lockManager->lock(new LockContext($file, ILock::TYPE_USER, self::USER1)); + self::assertSame(1, $this->lockRowCount($id)); + + $file->delete(); + self::assertSame(0, $this->lockRowCount($id)); + } + + /** + * Two mechanisms drop the lock of a deleted file: the storage wrapper and the + * event listener. Each has to work on its own, so that a change to one of them + * cannot leave a deleted file's lock behind. + */ + public function testStorageDeletionAloneRemovesTheLock(): void { + $file = $this->loginAndGetUserFolder(self::USER1)->newFile('storage-delete.txt', 'AAA'); + $id = $file->getId(); + $this->lockManager->lock(new LockContext($file, ILock::TYPE_USER, self::USER1)); + self::assertSame(1, $this->lockRowCount($id)); + + // straight to the storage, so no node event is dispatched for this deletion + self::assertTrue($file->getStorage()->unlink($file->getInternalPath())); + self::assertSame(0, $this->lockRowCount($id), 'the storage wrapper drops the lock by itself'); + } + + public function testNodeDeletedEventAloneRemovesTheLock(): void { + $file = $this->loginAndGetUserFolder(self::USER1)->newFile('event-delete.txt', 'AAA'); + $id = $file->getId(); + $this->lockManager->lock(new LockContext($file, ILock::TYPE_USER, self::USER1)); + self::assertSame(1, $this->lockRowCount($id)); + + // the file itself is left alone; only the event fires + \OCP\Server::get(IEventDispatcher::class)->dispatchTyped(new NodeDeletedEvent($file)); + self::assertSame(0, $this->lockRowCount($id), 'the listener drops the lock by itself'); + } + + /** + * Oracle refuses more than 1000 expressions in one IN list, so bulk removal + * has to be chunked. + */ + public function testBulkRemovalHandlesMoreIdsThanOneStatementAllows(): void { + $file = $this->loginAndGetUserFolder(self::USER1)->newFile('bulk-delete.txt', 'AAA'); + $id = $file->getId(); + $this->lockManager->lock(new LockContext($file, ILock::TYPE_USER, self::USER1)); + + $this->lockService()->removeLocksForFileIds(array_merge(range(900000, 901500), [$id])); + self::assertSame(0, $this->lockRowCount($id)); + } + + public function testRestoredFileIsNotLocked(): void { + $file = $this->loginAndGetUserFolder(self::USER1)->newFile('restored.txt', 'AAA'); + $id = $file->getId(); + $this->lockManager->lock(new LockContext($file, ILock::TYPE_USER, self::USER1)); + $file->delete(); + + $trashed = null; + foreach (Helper::getTrashFiles('/', self::USER1) as $item) { + if ($item->getName() === 'restored.txt') { + $trashed = $item; + } + } + if ($trashed === null) { + self::markTestSkipped('trash bin did not keep the file'); + } + self::assertTrue(Trashbin::restore('/restored.txt.d' . $trashed->getMtime(), 'restored.txt', $trashed->getMtime())); + + $this->lockService()->clearCache(); + $restored = $this->rootFolder->getUserFolder(self::USER1)->get('restored.txt'); + self::assertSame($id, $restored->getId()); + self::assertSame([], $this->lockManager->getLocks($id)); + self::assertSame(0, $this->lockRowCount($id)); + } + + public function testPurgingTheCacheEntryRemovesTheLock(): void { + $file = $this->loginAndGetUserFolder(self::USER1)->newFile('purged.txt', 'AAA'); + $id = $file->getId(); + $file->delete(); + + // a lock that somehow survived the deletion (created before this version) + $lock = new FileLock(); + $lock->setUserId(self::USER1)->setLockType(ILock::TYPE_USER)->setFileId($id)->setToken('files_lock/stale'); + \OCP\Server::get(LocksRequest::class)->save($lock); + self::assertSame(1, $this->lockRowCount($id)); + + $trashed = null; + foreach (Helper::getTrashFiles('/', self::USER1) as $item) { + if ($item->getName() === 'purged.txt') { + $trashed = $item; + } + } + if ($trashed === null) { + self::markTestSkipped('trash bin did not keep the file'); + } + Trashbin::delete('purged.txt', self::USER1, $trashed->getMtime()); + self::assertSame(0, $this->lockRowCount($id)); + } +} diff --git a/tests/Feature/WritePolicyTest.php b/tests/Feature/WritePolicyTest.php new file mode 100644 index 00000000..48dedd8e --- /dev/null +++ b/tests/Feature/WritePolicyTest.php @@ -0,0 +1,186 @@ +setUser(null); + } else { + \OC_User::setUserId($userId); + } + $this->lockService()->clearCache(); + } + + private function assertBlocked(File $file): void { + try { + $file->putContent('blocked'); + self::fail('write should have been blocked by the lock'); + } catch (ManuallyLockedException) { + } + } + + private function assertWritable(File $file, string $content): void { + $file->putContent($content); + self::assertSame($content, $file->getContent()); + } + + /** + * @return array{File, File, File} the file as seen by USER1 (owner), USER2 (writer) and USER3 (read-only) + */ + private function homeFile(string $name): array { + $owner = $this->sharedFile($name, 19, 1); + \OC_Util::setupFS(self::USER2); + \OC_Util::setupFS(self::USER3); + $writer = $this->rootFolder->getUserFolder(self::USER2)->get($name); + $reader = $this->rootFolder->getUserFolder(self::USER3)->get($name); + return [$owner, $writer, $reader]; + } + + /** + * @return array{File, File} the file as seen by USER1 and USER2 on a shared non-home mount + */ + private function mountedFile(string $name): array { + $storage = new Temporary([]); + $this->loginAndGetUserFolder(self::USER1); + \OC_Util::setupFS(self::USER2); + Filesystem::mount($storage, [], '/' . self::USER1 . '/files/ext/'); + Filesystem::mount($storage, [], '/' . self::USER2 . '/files/ext/'); + $file = $this->rootFolder->getUserFolder(self::USER1)->get('ext')->newFile($name, 'AAA'); + $other = $this->rootFolder->getUserFolder(self::USER2)->get('ext/' . $name); + self::assertSame($file->getId(), $other->getId()); + return [$file, $other]; + } + + public function testUserLockOnHomeFile(): void { + [$owner, $writer, $reader] = $this->homeFile('user-home.txt'); + $this->lockManager->lock(new LockContext($owner, ILock::TYPE_USER, self::USER1)); + + $this->actAs(self::USER1); + $this->assertWritable($owner, 'owner'); + $this->actAs(self::USER2); + $this->assertBlocked($writer); + $this->actAs(self::USER3); + $this->expectException(NotPermittedException::class); + $reader->putContent('reader'); + } + + public function testUserLockOnNonHomeMount(): void { + [$file, $other] = $this->mountedFile('user-ext.txt'); + $this->lockManager->lock(new LockContext($file, ILock::TYPE_USER, self::USER1)); + + $this->actAs(self::USER2); + self::assertCount(1, $this->lockManager->getLocks($other->getId())); + $this->assertBlocked($other); + $this->actAs(self::USER1); + $this->assertWritable($file, 'owner'); + } + + public function testAppLockOnHomeAndMount(): void { + [$owner, $writer] = $this->homeFile('app-home.txt'); + $scope = new LockContext($owner, ILock::TYPE_APP, 'text'); + $this->lockManager->lock($scope); + + $this->actAs(self::USER1); + $this->assertBlocked($owner); + $this->actAs(self::USER2); + $this->assertBlocked($writer); + $this->lockManager->runInScope($scope, fn () => $this->assertWritable($writer, 'in scope')); + $this->lockManager->runInScope(new LockContext($owner, ILock::TYPE_APP, 'other'), fn () => $this->assertBlocked($writer)); + + [$file, $other] = $this->mountedFile('app-ext.txt'); + $scope = new LockContext($file, ILock::TYPE_APP, 'text'); + $this->lockManager->lock($scope); + $this->actAs(self::USER2); + $this->assertBlocked($other); + $this->lockManager->runInScope($scope, fn () => $this->assertWritable($other, 'in scope')); + } + + public function testTokenLockRequiresTokenAndPrincipal(): void { + [$owner, $writer, $reader] = $this->homeFile('token-home.txt'); + $this->actAs(self::USER1); + $lock = $this->lockService()->acquire(new LockContext($owner, ILock::TYPE_TOKEN, self::USER1), null, self::TOKEN); + $tokenScope = new LockContext($owner, ILock::TYPE_TOKEN, $lock->getToken()); + + // creator without the token + $this->assertBlocked($owner); + // creator presenting the token through the lock scope + $this->lockManager->runInScope($tokenScope, fn () => $this->assertWritable($owner, 'creator with token')); + // creator presenting the token the way the WebDAV adapter does + $this->lockService()->presentToken($lock->getToken()); + $this->assertWritable($owner, 'creator via presented token'); + + // another writer, with and without the token + $this->actAs(self::USER2); + $this->assertBlocked($writer); + $this->lockManager->runInScope($tokenScope, fn () => $this->assertBlocked($writer)); + + // file owner is the creator here; a read-only user is stopped by permissions + $this->actAs(self::USER3); + try { + $reader->putContent('reader'); + self::fail('read-only user must not write'); + } catch (NotPermittedException) { + } + + // a sessionless trusted caller presenting the token acts for the creator + $this->actAs(''); + $this->lockManager->runInScope($tokenScope, fn () => $this->assertWritable($owner, 'internal with token')); + $this->assertBlocked($owner); + } + + public function testTokenLockCreatedByRecipientBlocksTheFileOwner(): void { + [$owner, $writer] = $this->homeFile('token-recipient.txt'); + $this->actAs(self::USER2); + $lock = $this->lockService()->acquire(new LockContext($writer, ILock::TYPE_TOKEN, self::USER2), null, self::TOKEN); + $tokenScope = new LockContext($writer, ILock::TYPE_TOKEN, $lock->getToken()); + + $this->actAs(self::USER1); + $this->assertBlocked($owner); + $this->lockManager->runInScope($tokenScope, fn () => $this->assertBlocked($owner)); + + $this->actAs(self::USER2); + $this->lockManager->runInScope($tokenScope, fn () => $this->assertWritable($writer, 'recipient with token')); + } + + public function testTokenLockOnNonHomeMount(): void { + [$file, $other] = $this->mountedFile('token-ext.txt'); + $this->actAs(self::USER1); + $lock = $this->lockService()->acquire(new LockContext($file, ILock::TYPE_TOKEN, self::USER1), null, self::TOKEN); + $tokenScope = new LockContext($file, ILock::TYPE_TOKEN, $lock->getToken()); + + $this->actAs(self::USER2); + $this->assertBlocked($other); + $this->lockManager->runInScope($tokenScope, fn () => $this->assertBlocked($other)); + $this->actAs(self::USER1); + $this->assertBlocked($file); + $this->lockManager->runInScope($tokenScope, fn () => $this->assertWritable($file, 'creator with token')); + } +} From c2270f05a21032b8d94087ff265105a3eddc4815 Mon Sep 17 00:00:00 2001 From: Git'Fellow <12234510+solracsf@users.noreply.github.com> Date: Sun, 6 Sep 2026 17:37:14 +0200 Subject: [PATCH 4/8] feat(dav): rebuild the native WebDAV adapter on the shared policy The lock backend created a lock and then updated it with the identity, the display name and the timeout, so a rejected second write left a row owned by a random token behind, the client's own text was stored and shown to everyone as the lock owner, and a missing Timeout header produced a lock that never expires. Sabre's token check was left as it is, which authorizes a write on possession of the token alone, and the requester's own user lock was hidden from the protocol so its holder could not use it. Errors came back as 500. - the backend builds the whole lock and stores it once, so a refused request leaves nothing behind; the display name comes from the user, and the timeout from the request or from the configured default - the plugin answers Sabre's token validation from the policy, so a token authorizes a write only for the principal the lock belongs to, and a collection operation is checked against the locks below it - a user lock is visible to its own holder again, and the responses carry a valid timeout and 423, 409 or 403 instead of 500 - only native WebDAV may lock a collection, as RFC 4918 asks; every other path refuses a folder - a listing of a DAV backed mount warms the remote properties once instead of asking the remote server about every file Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com> --- README.md | 2 +- lib/DAV/LockBackend.php | 154 +++++++----- lib/DAV/LockPlugin.php | 182 ++++++++++++--- lib/Service/FileService.php | 68 +----- lib/Service/LockService.php | 31 ++- tests/Feature/AcquisitionTest.php | 7 + tests/Feature/DavLockTest.php | 376 ++++++++++++++++++++++++++++++ tests/psalm-baseline.xml | 3 +- 8 files changed, 660 insertions(+), 163 deletions(-) create mode 100644 tests/Feature/DavLockTest.php diff --git a/README.md b/README.md index 7fdc8991..ea4d6292 100644 --- a/README.md +++ b/README.md @@ -130,7 +130,7 @@ WebDAV returns the following additional properties in response to a `PROPFIND` r - `{http://nextcloud.org/ns}lock-owner-displayname`: Display name of the lock owner - `{http://nextcloud.org/ns}lock-owner-editor`: App ID for an app-owned lock. Clients can use it to suggest joining the collaborative editing session in the web interface or through direct editing. In the response to an `X-User-Lock` `LOCK` request, this property currently contains the lock owner regardless of lock type. - `{http://nextcloud.org/ns}lock-time`: Timestamp at which the lock was created -- `{http://nextcloud.org/ns}lock-timeout`: Configured lock timeout in seconds from creation. A value of `0` indicates that the lock does not expire. +- `{http://nextcloud.org/ns}lock-timeout`: Lifetime of the lock in seconds counted from `lock-time`; it grows when the lock is refreshed. A value of `0` indicates that the lock does not expire. - `{http://nextcloud.org/ns}lock-token`: Lock token. Clients using native WebDAV locking must retain it while holding the lock and provide it when unlocking. ```bash diff --git a/lib/DAV/LockBackend.php b/lib/DAV/LockBackend.php index a21d63bc..a197ff06 100644 --- a/lib/DAV/LockBackend.php +++ b/lib/DAV/LockBackend.php @@ -9,23 +9,36 @@ namespace OCA\FilesLock\DAV; -use Exception; -use OCA\FilesLock\Service\FileService; +use Closure; +use OCA\FilesLock\Exceptions\LockNotFoundException; +use OCA\FilesLock\Exceptions\NotFileException; +use OCA\FilesLock\Exceptions\UnauthorizedUnlockException; +use OCA\FilesLock\Model\FileLock; use OCA\FilesLock\Service\LockService; +use OCP\Files\Folder; use OCP\Files\Lock\ILock; use OCP\Files\Lock\LockContext; use OCP\Files\Lock\OwnerLockedException; use OCP\Files\Node; -use OCP\Files\NotFoundException; use OCP\IUserSession; +use Sabre\DAV\Exception\Forbidden; +use Sabre\DAV\Exception\Locked; +use Sabre\DAV\Exception\NotFound; use Sabre\DAV\Locks\Backend\BackendInterface; use Sabre\DAV\Locks\LockInfo; +/** + * Thin adapter between Sabre's lock backend contract and the canonical lock + * service. Every lock stored by the app is presented to Sabre as an exclusive + * write lock, so Sabre's token validation applies to all lock types. + */ class LockBackend implements BackendInterface { + /** + * @param Closure(string): Node $nodeResolver resolves a request uri to a node, throws Sabre NotFound + */ public function __construct( - private readonly FileService $fileService, private readonly LockService $lockService, - private readonly bool $absolute, + private readonly Closure $nodeResolver, private readonly IUserSession $userSession, ) { } @@ -36,86 +49,117 @@ public function __construct( */ #[\Override] public function getLocks($uri, $returnChildLocks): array { - $locks = []; - try { - // TODO: check parent - $file = $this->getFileFromUri($uri); - $lock = $this->lockService->getLockFromFileId($file->getId()); + return array_map( + fn (FileLock $lock): LockInfo => $lock->toLockInfo(), + $this->getFileLocks($uri, (bool)$returnChildLocks) + ); + } - if ($lock->getType() === ILock::TYPE_USER && $lock->getOwner() === $this->userSession->getUser()?->getUID()) { - return []; - } + /** + * Active locks of the resource at $uri, optionally including locks on files below it. + * + * @return list + */ + public function getFileLocks(string $uri, bool $returnChildLocks): array { + try { + $node = $this->resolve($uri); + } catch (NotFound) { + return []; + } + $locks = []; + $lock = $this->lockService->getActiveLock($node->getId()); + if ($lock !== null) { $lock->setUri($uri); + $locks[] = $lock; + } - return [$lock->toLockInfo()]; - } catch (Exception) { - return $locks; + if ($returnChildLocks && $node instanceof Folder) { + foreach ($this->lockService->getLocksBelow($node->getId()) as $entry) { + $entry['lock']->setUri(rtrim($uri, '/') . '/' . $entry['path']); + $locks[] = $entry['lock']; + } } + + return $locks; } /** - * Locks a uri - * + * Create or refresh a token lock. The complete lock is built before it is + * persisted, so a refused request never leaves a partial row behind. * + * @throws Locked when another lock is held on the resource + * @throws Forbidden when the caller may not lock the resource */ #[\Override] public function lock($uri, LockInfo $lockInfo): bool { - try { - $file = $this->getFileFromUri($uri); - $lock = $this->lockService->lock(new LockContext( - $file, - ILock::TYPE_TOKEN, - $lockInfo->token - )); - - $user = $this->userSession->getUser(); - if ($user === null) { - return false; + $node = $this->resolve($uri); + $user = $this->userSession->getUser(); + if ($user === null) { + throw new Forbidden('Locking requires an authenticated user'); + } + + $timeout = null; + if ($lockInfo->timeout !== null) { + $timeout = $lockInfo->timeout === LockInfo::TIMEOUT_INFINITE ? FileLock::ETA_INFINITE : max(0, (int)$lockInfo->timeout); + if ($timeout === 0) { + $timeout = FileLock::ETA_INFINITE; } + } - $lock->setUserId($user->getUID()); - $lock->setTimeout($lockInfo->timeout ?? 0); - $lock->setToken($lockInfo->token); - $lock->setDisplayName($lockInfo->owner); - $lock->setScope($lockInfo->scope); - $this->lockService->update($lock); - return true; - } catch (NotFoundException) { - return true; - } catch (OwnerLockedException) { - return false; + try { + $lock = $this->lockService->acquire( + new LockContext($node, ILock::TYPE_TOKEN, $user->getUID()), + $timeout, + $lockInfo->token, + null, + false + ); + } catch (OwnerLockedException $e) { + /** @var FileLock $existing */ + $existing = $e->getLock(); + $existing->setUri($uri); + throw new Locked($existing->toLockInfo()); + } catch (NotFileException|UnauthorizedUnlockException $e) { + throw new Forbidden($e->getMessage()); } + + $lockInfo->token = $lock->getToken(); + $lockInfo->owner = $lock->getDisplayName(); + $lockInfo->created = $lock->getCreatedAt(); + $lockInfo->timeout = $lock->isInfinite() ? LockInfo::TIMEOUT_INFINITE : $lock->getETA(); + $lockInfo->depth = 0; + $lockInfo->uri = $uri; + return true; } /** * Removes a lock from a uri * - * + * @throws Forbidden when the presented token or the caller may not release the lock */ #[\Override] public function unlock($uri, LockInfo $lockInfo): bool { try { - $file = $this->getFileFromUri($uri); - } catch (NotFoundException) { + $node = $this->resolve($uri); + } catch (NotFound) { + return true; + } + $owner = $this->userSession->getUser()?->getUID() ?? $lockInfo->token; + try { + $this->lockService->unlock(new LockContext($node, ILock::TYPE_TOKEN, $owner), false, $lockInfo->token); + } catch (LockNotFoundException) { return true; + } catch (UnauthorizedUnlockException $e) { + throw new Forbidden($e->getMessage()); } - $this->lockService->unlock(new LockContext( - $file, - ILock::TYPE_TOKEN, - $lockInfo->token - )); return true; } /** - * @throws NotFoundException + * @throws NotFound */ - private function getFileFromUri(string $uri): Node { - if ($this->absolute) { - return $this->fileService->getFileFromAbsoluteUri($uri); - } - - return $this->fileService->getFileFromUri($uri); + private function resolve(string $uri): Node { + return ($this->nodeResolver)($uri); } } diff --git a/lib/DAV/LockPlugin.php b/lib/DAV/LockPlugin.php index b2ceccaa..3b6f41d3 100644 --- a/lib/DAV/LockPlugin.php +++ b/lib/DAV/LockPlugin.php @@ -7,17 +7,16 @@ namespace OCA\FilesLock\DAV; -use OCA\DAV\Connector\Sabre\CachingTree; use OCA\DAV\Connector\Sabre\Directory; use OCA\DAV\Connector\Sabre\FakeLockerPlugin; use OCA\DAV\Connector\Sabre\File; use OCA\DAV\Connector\Sabre\FilesPlugin; -use OCA\DAV\Connector\Sabre\ObjectTree; +use OCA\DAV\Connector\Sabre\Node as SabreNode; use OCA\FilesLock\AppInfo\Application; use OCA\FilesLock\Exceptions\LockNotFoundException; +use OCA\FilesLock\Exceptions\NotFileException; use OCA\FilesLock\Exceptions\UnauthorizedUnlockException; use OCA\FilesLock\Model\FileLock; -use OCA\FilesLock\Service\FileService; use OCA\FilesLock\Service\LockService; use OCP\AppFramework\Http; use OCP\Files\Lock\ILock; @@ -25,7 +24,9 @@ use OCP\Files\Lock\OwnerLockedException; use OCP\Files\Node; use OCP\IUserSession; -use Sabre\DAV\Exception\LockTokenMatchesRequestUri; +use Sabre\DAV\Exception\Forbidden; +use Sabre\DAV\Exception\Locked; +use Sabre\DAV\Exception\NotFound; use Sabre\DAV\INode; use Sabre\DAV\Locks\Plugin as SabreLockPlugin; use Sabre\DAV\PropFind; @@ -34,9 +35,10 @@ use Sabre\HTTP\ResponseInterface; class LockPlugin extends SabreLockPlugin { + private const array SUPPORTED_LOCK_TYPES = [ILock::TYPE_USER, ILock::TYPE_APP, ILock::TYPE_TOKEN]; + public function __construct( private readonly LockService $lockService, - private readonly FileService $fileService, private readonly IUserSession $userSession, ) { } @@ -51,21 +53,28 @@ public function initialize(Server $server): void { $server->removeListener('validateTokens', [$fakePlugin, 'validateTokens']); } - $absolute = false; - switch ($server->tree::class) { - case ObjectTree::class: - $absolute = false; - break; - - case CachingTree::class: - $absolute = true; - break; - } - $this->locksBackend = new LockBackend($this->fileService, $this->lockService, $absolute, $this->userSession); + $this->locksBackend = new LockBackend( + $this->lockService, + fn (string $uri): Node => $this->resolveNode($uri), + $this->userSession, + ); $server->on('propFind', $this->customProperties(...)); parent::initialize($server); } + /** + * Resolve a request uri through the DAV tree, whichever tree the server uses. + * + * @throws NotFound + */ + private function resolveNode(string $uri): Node { + $node = $this->server->tree->getNodeForPath($uri); + if (!$node instanceof SabreNode) { + throw new NotFound('Resource is not a file system node'); + } + return $node->getNode(); + } + private function cacheDirectory(Directory $directory): void { $children = $directory->getChildren(); @@ -80,12 +89,13 @@ private function cacheDirectory(Directory $directory): void { continue; } - $ids[] = (string)$id; + $ids[] = (int)$id; } - $ids[] = (string)$directory->getId(); + $ids[] = (int)$directory->getId(); // the lock service will take care of the caching $this->lockService->getLockForNodeIds($ids); + $this->lockService->prefetchRemoteLocks($directory->getNode()); } public function customProperties(PropFind $propFind, INode $node): void { @@ -182,14 +192,97 @@ public function customProperties(PropFind $propFind, INode $node): void { }); } + /** + * Replace Sabre's token-only validation with the application policy: a lock + * blocks a modifying request unless the acting principal may write the file + * (owner of a user lock, or owner of a token lock presenting its token). + * + * @param mixed $conditions + */ + #[\Override] + public function validateTokens(RequestInterface $request, &$conditions): void { + $this->lockService->resetPresentedTokens(); + foreach ($conditions as $condition) { + foreach ($condition['tokens'] as $token) { + if (str_starts_with((string)$token['token'], 'opaquelocktoken:')) { + $this->lockService->presentToken(substr((string)$token['token'], 16)); + } + } + } + + $method = $request->getMethod(); + if ($method === 'LOCK') { + parent::validateTokens($request, $conditions); + return; + } + + /** @var LockBackend $backend */ + $backend = $this->locksBackend; + $mustLocks = []; + switch ($method) { + case 'DELETE': + $mustLocks = $backend->getFileLocks($request->getPath(), true); + break; + case 'MKCOL': + case 'MKCALENDAR': + case 'PROPPATCH': + case 'PUT': + case 'PATCH': + $mustLocks = $backend->getFileLocks($request->getPath(), false); + break; + case 'MOVE': + $mustLocks = array_merge( + $backend->getFileLocks($request->getPath(), true), + $backend->getFileLocks($this->server->calculateUri($request->getHeader('Destination')), false) + ); + break; + case 'COPY': + $mustLocks = $backend->getFileLocks($this->server->calculateUri($request->getHeader('Destination')), false); + break; + } + + $byToken = []; + foreach ($mustLocks as $lock) { + $byToken[$lock->getToken()] = $lock; + } + + foreach ($conditions as $kk => $condition) { + foreach ($condition['tokens'] as $ii => $token) { + if (!str_starts_with((string)$token['token'], 'opaquelocktoken:')) { + continue; + } + $checkToken = substr((string)$token['token'], 16); + if (isset($byToken[$checkToken])) { + $conditions[$kk]['tokens'][$ii]['validToken'] = true; + continue; + } + foreach ($backend->getFileLocks($condition['uri'], false) as $oddLock) { + if ($oddLock->getToken() === $checkToken) { + $conditions[$kk]['tokens'][$ii]['validToken'] = true; + continue 2; + } + } + } + } + + foreach ($byToken as $lock) { + if (!$this->lockService->canWrite($lock, null)) { + throw new Locked($lock->toLockInfo()); + } + } + } + #[\Override] public function httpLock(RequestInterface $request, ResponseInterface $response) { if ($request->getHeader('X-User-Lock')) { - /** @var ILock::TYPE_* $lockType */ - $lockType = (int)($request->getHeader('X-User-Lock-Type') ?? ILock::TYPE_USER); + $lockType = $this->getRequestedLockType($request); $response->setHeader('Content-Type', 'application/xml; charset=utf-8'); - $file = $this->fileService->getFileFromAbsoluteUri($this->server->getRequestUri()); + $file = $this->resolveNode($this->server->getRequestUri()); + $user = $this->userSession->getUser(); + if ($user === null) { + throw new Forbidden('Locking requires an authenticated user'); + } $user = $this->userSession->getUser(); if ($user === null) { @@ -197,7 +290,7 @@ public function httpLock(RequestInterface $request, ResponseInterface $response) } try { - $lockInfo = $this->lockService->lock(new LockContext( + $lockInfo = $this->lockService->acquire(new LockContext( $file, $lockType, $user->getUID() )); $response->setStatus(200); @@ -208,13 +301,16 @@ public function httpLock(RequestInterface $request, ResponseInterface $response) ) ); } catch (OwnerLockedException $e) { + $existing = $e->getLock(); $response->setStatus(423); $response->setBody( $this->server->xml->write( '{DAV:}prop', - $this->getLockProperties($e->getLock(), $file) + $this->getLockProperties($existing instanceof FileLock ? $existing : null, $file) ) ); + } catch (NotFileException|UnauthorizedUnlockException $e) { + throw new Forbidden($e->getMessage()); } return false; @@ -226,16 +322,18 @@ public function httpLock(RequestInterface $request, ResponseInterface $response) #[\Override] public function httpUnlock(RequestInterface $request, ResponseInterface $response) { if ($request->getHeader('X-User-Lock')) { - /** @var ILock::TYPE_* $lockType */ - $lockType = (int)($request->getHeader('X-User-Lock-Type') ?? ILock::TYPE_USER); + $lockType = $this->getRequestedLockType($request); $response->setHeader('Content-Type', 'application/xml; charset=utf-8'); - $file = $this->fileService->getFileFromAbsoluteUri($this->server->getRequestUri()); + $file = $this->resolveNode($this->server->getRequestUri()); + $user = $this->userSession->getUser(); + if ($user === null) { + throw new Forbidden('Unlocking requires an authenticated user'); + } try { - $this->lockService->enableUserOverride(); $this->lockService->unlock(new LockContext( - $file, $lockType, $this->userSession->getUser()->getUID() + $file, $lockType, $user->getUID() )); $response->setStatus(200); $response->setBody( @@ -253,7 +351,7 @@ public function httpUnlock(RequestInterface $request, ResponseInterface $respons ) ); } catch (UnauthorizedUnlockException) { - $lock = $this->lockService->getLockFromFileId($file->getId()); + $lock = $this->lockService->getActiveLock($file->getId()); $response->setStatus(Http::STATUS_LOCKED); $response->setBody( $this->server->xml->write( @@ -266,20 +364,28 @@ public function httpUnlock(RequestInterface $request, ResponseInterface $respons return false; } - try { - return parent::httpUnlock($request, $response); - } catch (LockTokenMatchesRequestUri) { - // Skip logging with wrong lock token - return false; + return parent::httpUnlock($request, $response); + } + + private function getRequestedLockType(RequestInterface $request): int { + $header = $request->getHeader('X-User-Lock-Type'); + if ($header === null || $header === '') { + return ILock::TYPE_USER; } + if (!is_numeric($header) || !in_array((int)$header, self::SUPPORTED_LOCK_TYPES, true)) { + throw new \Sabre\DAV\Exception\BadRequest('Unsupported lock type'); + } + return (int)$header; } private function getLockProperties(?FileLock $lock, Node $file): array { - // We need to fetch the node again to get the proper new Etag - $actingUser = ($file->getOwner() ? $file->getOwner()->getUID() : null) ?? $this->userSession->getUser()->getUID(); - $file = $this->fileService->getFileFromId($actingUser, $file->getId()); + if ($lock !== null) { + $this->lockService->injectMetadata($lock); + } + // the lock change updated the etag in the cache, read it back from there + $etag = $file->getStorage()->getCache()->get($file->getInternalPath())?->getEtag() ?? $file->getEtag(); return [ - FilesPlugin::GETETAG_PROPERTYNAME => $file->getEtag(), + FilesPlugin::GETETAG_PROPERTYNAME => $etag, Application::DAV_PROPERTY_LOCK => $lock !== null, Application::DAV_PROPERTY_LOCK_OWNER_TYPE => $lock ? $lock->getType() : null, Application::DAV_PROPERTY_LOCK_OWNER => $lock ? $lock->getOwner() : null, diff --git a/lib/Service/FileService.php b/lib/Service/FileService.php index b0217219..9610cde3 100644 --- a/lib/Service/FileService.php +++ b/lib/Service/FileService.php @@ -12,9 +12,6 @@ use OCP\Files\IRootFolder; use OCP\Files\Node; use OCP\Files\NotFoundException; -use OCP\Files\NotPermittedException; -use OCP\IUserSession; -use OCP\Session\Exceptions\SessionNotAvailableException; /** * Class FileService @@ -23,78 +20,21 @@ */ class FileService { public function __construct( - private readonly IUserSession $userSession, private readonly IRootFolder $rootFolder, ) { } /** + * Resolve a file id from the point of view of a user. * * @throws NotFoundException */ public function getFileFromId(string $userId, int $fileId): Node { - $files = $this->rootFolder->getUserFolder($userId) - ->getById($fileId); - - if (sizeof($files) === 0) { - throw new NotFoundException(); - } - - return array_shift($files); - } - - /** - * - * @throws NotFoundException - */ - public function getFileFromPath(string $userId, string $path): Node { - if (!str_starts_with($path, 'files/')) { - throw new NotFoundException(); - } - - $path = '/' . substr($path, 6); - - return $this->rootFolder->getUserFolder($userId) - ->get($path); - } - - /** - * @throws NotFoundException - */ - public function getFileFromUri(string $uri): Node { - $user = $this->userSession->getUser(); - if (is_null($user)) { - throw new SessionNotAvailableException(); - } - - $userId = $user->getUID(); - - $path = '/' . $uri; - - return $this->rootFolder->getUserFolder($userId) - ->get($path); - } - - /** - * - * @throws NotFoundException - * @throws NotPermittedException - */ - public function getFileFromAbsoluteUri(string $uri): Node { - $user = $this->userSession->getUser(); - if ($user === null) { - throw new SessionNotAvailableException(); - } - - $userId = $user->getUID(); - - [$root, , $path] = explode('/', trim($uri, '/') . '/', 3); - if ($root !== 'files') { + $node = $this->rootFolder->getUserFolder($userId)->getFirstNodeById($fileId); + if ($node === null) { throw new NotFoundException(); } - $path = '/' . $path; - return $this->rootFolder->getUserFolder($userId) - ->get($path); + return $node; } } diff --git a/lib/Service/LockService.php b/lib/Service/LockService.php index f1f10d70..9ef5451f 100644 --- a/lib/Service/LockService.php +++ b/lib/Service/LockService.php @@ -17,6 +17,7 @@ use OCA\FilesLock\Db\LocksRequest; use OCA\FilesLock\Exceptions\LockConflictException; use OCA\FilesLock\Exceptions\LockNotFoundException; +use OCA\FilesLock\Exceptions\NotFileException; use OCA\FilesLock\Exceptions\UnauthorizedUnlockException; use OCA\FilesLock\Model\FileLock; use OCP\App\IAppManager; @@ -24,6 +25,7 @@ use OCP\AppFramework\Utility\ITimeFactory; use OCP\Constants; use OCP\EventDispatcher\IEventDispatcher; +use OCP\Files\File; use OCP\Files\IHomeStorage; use OCP\Files\InvalidPathException; use OCP\Files\IRootFolder; @@ -213,12 +215,14 @@ public function lock(LockContext $lockScope): FileLock { * @param int|null $timeout lifetime in seconds, <= 0 for no expiry, null for the configured value * @param string|null $token token to record for a new lock (native WebDAV), generated when null * @param string|null $displayName display name to record, resolved from the owner when null + * @param bool $filesOnly refuse folders; native WebDAV may lock a collection (RFC 4918) * * @throws OwnerLockedException * @throws UnauthorizedUnlockException + * @throws NotFileException */ - public function acquire(LockContext $lockScope, ?int $timeout = null, ?string $token = null, ?string $displayName = null): FileLock { - $this->canLock($lockScope); + public function acquire(LockContext $lockScope, ?int $timeout = null, ?string $token = null, ?string $displayName = null, bool $filesOnly = true): FileLock { + $this->canLock($lockScope, null, $filesOnly); $fileId = $lockScope->getNode()->getId(); $timeout ??= $this->getConfiguredTimeout(); $now = $this->now(); @@ -333,7 +337,10 @@ public function enableUserOverride(): void { * @throws UnauthorizedUnlockException when the node cannot be locked by the caller * @throws NotFileException when the node is not a file */ - public function canLock(LockContext $request, ?FileLock $current = null): void { + public function canLock(LockContext $request, ?FileLock $current = null, bool $filesOnly = true): void { + if ($filesOnly && !$request->getNode() instanceof File) { + throw new NotFileException($this->l10n->t('Only files can be locked.')); + } if (($request->getNode()->getPermissions() & Constants::PERMISSION_UPDATE) === 0) { throw new UnauthorizedUnlockException( $this->l10n->t('File can only be locked with update permissions.') @@ -676,6 +683,24 @@ public function getRemoteLockFromDav(int $nodeId, ?Node $node = null): ?FileLock } } + /** + * Warm the remote property cache of a DAV backed folder with one remote + * listing so that per-file lookups do not trigger remote requests. + */ + public function prefetchRemoteLocks(Node $folder): void { + try { + $storage = $folder->getStorage(); + while ($storage->instanceOfStorage(Wrapper::class)) { + $storage = $storage->getWrapperStorage(); + } + if (!$storage->instanceOfStorage(DAV::class)) { + return; + } + } catch (\Exception $e) { + $this->logger->debug('Failed to prefetch remote locks: ' . $e->getMessage(), ['exception' => $e]); + } + } + private function propagateEtag(Node $node): void { try { $node->getStorage()->getCache()->update($node->getId(), [ diff --git a/tests/Feature/AcquisitionTest.php b/tests/Feature/AcquisitionTest.php index dc72e848..ed99bf10 100644 --- a/tests/Feature/AcquisitionTest.php +++ b/tests/Feature/AcquisitionTest.php @@ -11,6 +11,7 @@ use OCA\FilesLock\Db\LocksRequest; use OCA\FilesLock\Exceptions\LockConflictException; +use OCA\FilesLock\Exceptions\NotFileException; use OCA\FilesLock\Exceptions\UnauthorizedUnlockException; use OCA\FilesLock\Model\FileLock; use OCP\Files\Lock\ILock; @@ -86,6 +87,12 @@ public function testExpiredLockIsReplaced(): void { self::assertSame(1, $this->lockRowCount($file->getId())); } + public function testFoldersCannotBeLocked(): void { + $folder = $this->loginAndGetUserFolder(self::USER1)->newFolder('a-folder'); + $this->expectException(NotFileException::class); + $this->lockManager->lock(new LockContext($folder, ILock::TYPE_USER, self::USER1)); + } + public function testLockingNeedsUpdatePermission(): void { $file = $this->sharedFile('readonly.txt', 1); $shared = $this->loginAndGetUserFolder(self::USER2)->get('readonly.txt'); diff --git a/tests/Feature/DavLockTest.php b/tests/Feature/DavLockTest.php new file mode 100644 index 00000000..92edebfd --- /dev/null +++ b/tests/Feature/DavLockTest.php @@ -0,0 +1,376 @@ +user]; + } + + public function challenge(RequestInterface $request, ResponseInterface $response) { + } +} + +class CapturingSapi { + private ?Response $response = null; + + public function __construct( + private readonly Request $request, + ) { + } + + public function getRequest(): Request { + return $this->request; + } + + public function sendResponse(Response $response): void { + $copy = fopen('php://temp', 'r+'); + $body = $response->getBody(); + if (is_string($body)) { + fwrite($copy, $body); + } elseif (is_resource($body)) { + stream_copy_to_stream($body, $copy); + } elseif (is_callable($body)) { + ob_start(); + $body(); + fwrite($copy, (string)ob_get_clean()); + } + rewind($copy); + $this->response = new Response($response->getStatus(), $response->getHeaders(), $copy); + } + + public function getResponse(): Response { + return $this->response; + } +} + +/** + * Native WebDAV LOCK/UNLOCK, If-header validation and X-User-Lock through a + * real Sabre server built by the DAV app. + */ +#[Group(name: 'DB')] +class DavLockTest extends LockTestCase { + private const string LOCK_BODY = '%s'; + + private ServerFactory $serverFactory; + + protected function setUp(): void { + parent::setUp(); + $this->serverFactory = new ServerFactory( + \OCP\Server::get(IConfig::class), + \OCP\Server::get(LoggerInterface::class), + \OCP\Server::get(IDBConnection::class), + \OCP\Server::get(IUserSession::class), + \OCP\Server::get(IMountManager::class), + \OCP\Server::get(ITagManager::class), + $this->createMock(IRequest::class), + \OCP\Server::get(IPreview::class), + \OCP\Server::get(IEventDispatcher::class), + \OCP\Server::get(IFactory::class)->get('dav'), + ); + } + + /** + * @param array $headers + */ + private function request(string $user, string $method, string $path, ?string $body = null, array $headers = []): Response { + $this->loginAsUser($user); + $this->lockService()->clearCache(); + $view = new View('/' . $user . '/files'); + $server = $this->serverFactory->createServer(false, '/', 'dummy', new \Sabre\DAV\Auth\Plugin(new StaticAuthBackend($user)), fn (): \OC\Files\View => $view); + $stream = null; + if ($body !== null) { + $stream = fopen('php://temp', 'r+'); + fwrite($stream, $body); + rewind($stream); + } + $request = new Request($method, $path, $headers, $stream); + $sapi = new CapturingSapi($request); + $server->sapi = $sapi; + $server->httpRequest = $request; + $server->exec(); + // a refused request never reaches Sabre's afterMethod, so the transactional + // file lock taken for PUT stays behind; every real request is its own process + \OCP\Server::get(ILockingProvider::class)->releaseAll(); + $this->lockService()->clearCache(); + return $sapi->getResponse(); + } + + private function body(Response $response): string { + $body = $response->getBody(); + if (is_resource($body)) { + rewind($body); + return (string)stream_get_contents($body); + } + return (string)$body; + } + + private function nativeLock(string $user, string $path, string $owner = 'client', array $headers = []): Response { + return $this->request($user, 'LOCK', $path, sprintf(self::LOCK_BODY, $owner), $headers + ['Content-Type' => 'application/xml']); + } + + private function tokenOf(Response $response): string { + $header = (string)$response->getHeader('Lock-Token'); + self::assertMatchesRegularExpression('/^$/', $header); + return substr($header, strlen('request($user, 'PROPFIND', $path, '', ['Depth' => '0']); + self::assertSame(207, $response->getStatus()); + $body = $this->body($response); + $props = []; + foreach (['lock', 'lock-owner', 'lock-owner-type', 'lock-timeout', 'lock-token'] as $prop) { + preg_match('#([^<]*)#', $body, $m); + $props[$prop] = $m[1] ?? null; + } + $props['lockdiscovery'] = str_contains($body, ''); + return $props; + } + + public function testNativeLockLifecycle(): void { + $this->setLockTimeoutMinutes(-1); + $this->toTheFuture(0); + $file = $this->loginAndGetUserFolder(self::USER1)->newFile('native.txt', 'AAA'); + + $response = $this->nativeLock(self::USER1, '/native.txt'); + self::assertSame(200, $response->getStatus()); + $token = $this->tokenOf($response); + self::assertStringContainsString('Infinite', $this->body($response)); + $stored = $this->storedLock($file->getId()); + self::assertSame(self::USER1, $stored?->getOwner()); + self::assertSame(ILock::TYPE_TOKEN, $stored?->getType()); + self::assertSame($token, $stored?->getToken()); + self::assertNull($stored?->getExpiresAt()); + self::assertSame(self::USER1, $stored?->getDisplayName(), 'display name comes from the user, not from the client'); + + // refresh with a timeout + $response = $this->request(self::USER1, 'LOCK', '/native.txt', null, ['If' => '()', 'Timeout' => 'Second-1200']); + self::assertSame(200, $response->getStatus()); + self::assertStringContainsString('Second-1200', $this->body($response)); + self::assertSame($this->time + 1200, $this->storedLock($file->getId())?->getExpiresAt()); + self::assertSame(1, $this->lockRowCount($file->getId())); + + // creator writes with the token, not without it + self::assertSame(423, $this->request(self::USER1, 'PUT', '/native.txt', 'BBB')->getStatus()); + self::assertContains($this->request(self::USER1, 'PUT', '/native.txt', 'CCC', ['If' => '()'])->getStatus(), [200, 204]); + self::assertSame('CCC', $this->rootFolder->getUserFolder(self::USER1)->get('native.txt')->getContent()); + + // wrong token, then the right one + self::assertSame(409, $this->request(self::USER1, 'UNLOCK', '/native.txt', null, ['Lock-Token' => ''])->getStatus()); + self::assertSame(1, $this->lockRowCount($file->getId())); + self::assertSame(204, $this->request(self::USER1, 'UNLOCK', '/native.txt', null, ['Lock-Token' => ''])->getStatus()); + self::assertSame(0, $this->lockRowCount($file->getId())); + } + + public function testNativeTimeoutHeaders(): void { + $this->setLockTimeoutMinutes(30); + $this->toTheFuture(0); + $file = $this->loginAndGetUserFolder(self::USER1)->newFile('timeout.txt', 'AAA'); + + $response = $this->nativeLock(self::USER1, '/timeout.txt'); + self::assertStringContainsString('Second-1800', $this->body($response), 'no header falls back to the configured timeout'); + self::assertSame($this->time + 1800, $this->storedLock($file->getId())?->getExpiresAt()); + self::assertSame('1800', $this->lockProps(self::USER1, '/timeout.txt')['lock-timeout']); + $this->request(self::USER1, 'UNLOCK', '/timeout.txt', null, ['Lock-Token' => 'tokenOf($response) . '>']); + + $response = $this->nativeLock(self::USER1, '/timeout.txt', 'client', ['Timeout' => 'Second-600']); + self::assertStringContainsString('Second-600', $this->body($response)); + self::assertSame($this->time + 600, $this->storedLock($file->getId())?->getExpiresAt()); + self::assertSame('600', $this->lockProps(self::USER1, '/timeout.txt')['lock-timeout']); + $this->request(self::USER1, 'UNLOCK', '/timeout.txt', null, ['Lock-Token' => 'tokenOf($response) . '>']); + + $response = $this->nativeLock(self::USER1, '/timeout.txt', 'client', ['Timeout' => 'Infinite']); + self::assertStringContainsString('Infinite', $this->body($response)); + self::assertNull($this->storedLock($file->getId())?->getExpiresAt()); + // clients read 0 as "never expires"; a negative value lands in the past + self::assertSame('0', $this->lockProps(self::USER1, '/timeout.txt')['lock-timeout']); + } + + public function testOwnerMetadataIsNotPersisted(): void { + $file = $this->loginAndGetUserFolder(self::USER1)->newFile('owner.txt', 'AAA'); + $response = $this->nativeLock(self::USER1, '/owner.txt', 'Admin (Desktop client)'); + self::assertSame(200, $response->getStatus()); + self::assertSame(self::USER1, $this->storedLock($file->getId())?->getDisplayName()); + $this->request(self::USER1, 'UNLOCK', '/owner.txt', null, ['Lock-Token' => 'tokenOf($response) . '>']); + + $response = $this->nativeLock(self::USER1, '/owner.txt', str_repeat('x', 5000)); + self::assertSame(200, $response->getStatus()); + self::assertSame(1, $this->lockRowCount($file->getId())); + self::assertSame(self::USER1, $this->storedLock($file->getId())?->getDisplayName()); + } + + public function testForeignPrincipalCannotUseTheToken(): void { + $file = $this->sharedFile('foreign.txt'); + $response = $this->nativeLock(self::USER1, '/foreign.txt'); + $token = $this->tokenOf($response); + + self::assertSame($token, $this->lockProps(self::USER2, '/foreign.txt')['lock-token'], 'the token stays discoverable'); + self::assertSame(423, $this->request(self::USER2, 'PUT', '/foreign.txt', 'BBB')->getStatus()); + self::assertSame(423, $this->request(self::USER2, 'PUT', '/foreign.txt', 'BBB', ['If' => '()'])->getStatus(), 'RFC 4918 6.4: the principal must match the lock creator'); + self::assertSame('AAA', $this->rootFolder->getUserFolder(self::USER1)->get('foreign.txt')->getContent()); + self::assertSame(423, $this->request(self::USER2, 'MOVE', '/foreign.txt', null, ['Destination' => '/moved.txt', 'If' => '()'])->getStatus()); + self::assertSame(423, $this->request(self::USER2, 'DELETE', '/foreign.txt', null, ['If' => '()'])->getStatus()); + self::assertSame(1, $this->lockRowCount($file->getId())); + } + + public function testReadOnlyRecipientCannotReleaseWithThePublishedToken(): void { + $file = $this->sharedFile('ro-token.txt', 19, 1); + $response = $this->nativeLock(self::USER2, '/ro-token.txt'); + self::assertSame(200, $response->getStatus()); + $token = $this->tokenOf($response); + + // the token stays discoverable, as RFC 4918 section 6.5 allows + self::assertSame($token, $this->lockProps(self::USER3, '/ro-token.txt')['lock-token']); + + // but a user who may not write the file may not release its lock + self::assertSame(403, $this->request(self::USER3, 'UNLOCK', '/ro-token.txt', null, ['Lock-Token' => ''])->getStatus()); + self::assertSame(1, $this->lockRowCount($file->getId())); + + // the user it was created for still can + self::assertSame(204, $this->request(self::USER2, 'UNLOCK', '/ro-token.txt', null, ['Lock-Token' => ''])->getStatus()); + self::assertSame(0, $this->lockRowCount($file->getId())); + } + + public function testNativeLockOverExistingLockIsRefused(): void { + $file = $this->sharedFile('taken.txt'); + $this->lockManager->lock(new LockContext($file, ILock::TYPE_USER, self::USER1)); + $response = $this->nativeLock(self::USER2, '/taken.txt'); + self::assertSame(423, $response->getStatus()); + self::assertSame(ILock::TYPE_USER, $this->storedLock($file->getId())?->getType()); + self::assertSame(1, $this->lockRowCount($file->getId())); + + // the owner's own user lock is not a fake success either + $response = $this->nativeLock(self::USER1, '/taken.txt'); + self::assertSame(423, $response->getStatus()); + self::assertSame(ILock::TYPE_USER, $this->storedLock($file->getId())?->getType()); + } + + public function testUserLockCreatorHasTheNativeLifecycle(): void { + $file = $this->sharedFile('userlock.txt'); + $lock = $this->lockManager->lock(new LockContext($file, ILock::TYPE_USER, self::USER1)); + + $props = $this->lockProps(self::USER1, '/userlock.txt'); + self::assertSame('1', $props['lock']); + self::assertSame($lock->getToken(), $props['lock-token']); + self::assertTrue($props['lockdiscovery'], 'the creator sees the lock in lockdiscovery'); + + self::assertContains($this->request(self::USER1, 'PUT', '/userlock.txt', 'BBB')->getStatus(), [200, 204]); + self::assertContains($this->request(self::USER1, 'PUT', '/userlock.txt', 'CCC', ['If' => '(getToken() . '>)'])->getStatus(), [200, 204]); + self::assertSame(423, $this->request(self::USER2, 'PUT', '/userlock.txt', 'DDD')->getStatus()); + self::assertSame(403, $this->request(self::USER2, 'UNLOCK', '/userlock.txt', null, ['Lock-Token' => 'getToken() . '>'])->getStatus(), 'a user lock is bound to its user'); + self::assertSame(1, $this->lockRowCount($file->getId())); + self::assertSame(204, $this->request(self::USER1, 'UNLOCK', '/userlock.txt', null, ['Lock-Token' => 'getToken() . '>'])->getStatus()); + self::assertSame(0, $this->lockRowCount($file->getId())); + } + + public function testXUserLock(): void { + $file = $this->sharedFile('xuser.txt'); + $folder = $this->loginAndGetUserFolder(self::USER1)->newFolder('xfolder'); + + $response = $this->request(self::USER1, 'LOCK', '/xuser.txt', null, ['X-User-Lock' => '1']); + self::assertSame(200, $response->getStatus()); + self::assertStringContainsString('' . self::USER1 . '', $this->body($response)); + self::assertStringContainsString('0', $this->body($response)); + + $response = $this->request(self::USER2, 'LOCK', '/xuser.txt', null, ['X-User-Lock' => '1']); + self::assertSame(423, $response->getStatus()); + self::assertStringContainsString('' . self::USER1 . '', $this->body($response)); + + self::assertSame(423, $this->request(self::USER2, 'UNLOCK', '/xuser.txt', null, ['X-User-Lock' => '1'])->getStatus()); + self::assertSame(400, $this->request(self::USER1, 'UNLOCK', '/xuser.txt', null, ['X-User-Lock' => '1', 'X-User-Lock-Type' => '99'])->getStatus()); + self::assertSame(200, $this->request(self::USER1, 'UNLOCK', '/xuser.txt', null, ['X-User-Lock' => '1'])->getStatus()); + self::assertSame(412, $this->request(self::USER1, 'UNLOCK', '/xuser.txt', null, ['X-User-Lock' => '1'])->getStatus()); + self::assertSame(404, $this->request(self::USER1, 'LOCK', '/missing.txt', null, ['X-User-Lock' => '1'])->getStatus()); + self::assertSame(403, $this->request(self::USER1, 'LOCK', '/xfolder', null, ['X-User-Lock' => '1'])->getStatus()); + self::assertSame(0, $this->lockRowCount($folder->getId())); + + // desktop client style token lock: the creator releases it with the header, others do not + $response = $this->request(self::USER1, 'LOCK', '/xuser.txt', null, ['X-User-Lock' => '1', 'X-User-Lock-Type' => '2']); + self::assertSame(200, $response->getStatus()); + self::assertSame(ILock::TYPE_TOKEN, $this->storedLock($file->getId())?->getType()); + self::assertSame(423, $this->request(self::USER2, 'UNLOCK', '/xuser.txt', null, ['X-User-Lock' => '1', 'X-User-Lock-Type' => '2'])->getStatus()); + self::assertSame(200, $this->request(self::USER1, 'UNLOCK', '/xuser.txt', null, ['X-User-Lock' => '1', 'X-User-Lock-Type' => '2'])->getStatus()); + } + + public function testAncestorOperationsAreRefusedBeforeMutation(): void { + $root = $this->loginAndGetUserFolder(self::USER1); + $dir = $root->newFolder('tree'); + $locked = $dir->newFolder('sub')->newFile('locked.txt', 'important'); + $this->shareWith($dir, self::USER1, self::USER2, 31); + $this->lockManager->lock(new LockContext($locked, ILock::TYPE_USER, self::USER1)); + + self::assertSame(423, $this->request(self::USER2, 'DELETE', '/tree/sub')->getStatus()); + self::assertSame(423, $this->request(self::USER2, 'MOVE', '/tree/sub', null, ['Destination' => '/tree/sub2'])->getStatus()); + self::assertSame(423, $this->request(self::USER2, 'DELETE', '/tree/sub/locked.txt')->getStatus()); + + $this->loginAsUser(self::USER1); + $this->lockService()->clearCache(); + self::assertSame('important', $this->rootFolder->getUserFolder(self::USER1)->get('tree/sub/locked.txt')->getContent()); + self::assertSame(1, $this->lockRowCount($locked->getId())); + self::assertSame(0, count(\OCP\Server::get(LocksRequest::class)->getLocksBelow($dir->getId())) - 1); + } + + public function testNativeClientMayLockACollection(): void { + $folder = $this->loginAndGetUserFolder(self::USER1)->newFolder('coll'); + $folder->newFile('inside.txt', 'AAA'); + $response = $this->nativeLock(self::USER1, '/coll/'); + self::assertSame(200, $response->getStatus()); + $token = $this->tokenOf($response); + self::assertSame(1, $this->lockRowCount($folder->getId())); + self::assertSame(403, $this->request(self::USER1, 'LOCK', '/coll/', null, ['X-User-Lock' => '1'])->getStatus(), 'the application paths still refuse folders'); + self::assertSame(204, $this->request(self::USER1, 'UNLOCK', '/coll/', null, ['Lock-Token' => ''])->getStatus()); + self::assertSame(0, $this->lockRowCount($folder->getId())); + } + + public function testOtherUsersLockIsVisibleButNotUsable(): void { + $file = $this->sharedFile('visible.txt'); + $this->lockManager->lock(new LockContext($file, ILock::TYPE_USER, self::USER1)); + $props = $this->lockProps(self::USER2, '/visible.txt'); + self::assertSame('1', $props['lock']); + self::assertSame(self::USER1, $props['lock-owner']); + self::assertSame('0', $props['lock-owner-type']); + self::assertSame(423, $this->request(self::USER2, 'PROPPATCH', '/visible.txt', '1')->getStatus()); + } + + #[\Override] + protected function sharedFile(string $name, int $permissions = 19, ?int $permissionsUser3 = null): File { + return parent::sharedFile($name, $permissions, $permissionsUser3); + } +} diff --git a/tests/psalm-baseline.xml b/tests/psalm-baseline.xml index 7b4cfb5f..5f03c12b 100644 --- a/tests/psalm-baseline.xml +++ b/tests/psalm-baseline.xml @@ -2,12 +2,11 @@ - - + From dd65a544f28282f22a611b22f45a973b8161f247 Mon Sep 17 00:00:00 2001 From: Git'Fellow <12234510+solracsf@users.noreply.github.com> Date: Sun, 6 Sep 2026 17:38:26 +0200 Subject: [PATCH 5/8] fix(api): align the OCS and CLI contracts with what the app now does Both front ends still answered ordinary client mistakes with 500, ignored the lock type when releasing a lock, and reported failures by dumping an exception. The OCS conflict payload also carried the token of the lock it was reporting, which the caller has no use for because OCS never accepts one. - OCS validates the lock type and the file id, and answers 400, 403, 404, 412 or 423 instead of 500; the conflict payload no longer carries the token - the lock type given to an unlock is honoured, and the user a lock was recorded for can release it whatever the type - occ reports an already locked file, a folder, a missing file or an unknown user in one line and exits non-zero, and its forced unlock removes the lock by file id so it no longer depends on the lock owner still having access - the README describes the behaviour that is now implemented: what a token lock requires, how expiry is refreshed, which storages are covered, what the status codes mean, and that only native WebDAV may lock a collection Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com> --- README.md | 76 +++++++++++++++++++++-------- lib/Command/Lock.php | 38 +++++++++------ lib/Controller/LockController.php | 44 ++++++++++++++--- tests/Feature/CommandTest.php | 61 +++++++++++++++++++++-- tests/Feature/OcsControllerTest.php | 59 ++++++++++++++++++++++ 5 files changed, 231 insertions(+), 47 deletions(-) diff --git a/README.md b/README.md index ea4d6292..cc0e803e 100644 --- a/README.md +++ b/README.md @@ -30,6 +30,12 @@ Locks use an ownership and authorization model (**lock type**) and are created o Token-owned locks are primarily intended for automatic client locking, for example when a file is opened by a desktop client or another WebDAV client that supports native WebDAV locking. The lock type alone does not imply that a lock was created automatically. + A token-owned lock records both the token and the user who created it. Modifying the file requires the token and the request must come from that user (RFC 4918, section 6.4); a request from another user is refused even if it carries the token, and a write through the PHP file system API requires the token to be presented through the lock scope of `ILockManager`. The lock is released by a native `UNLOCK` that presents the token and comes from someone who may write the file, by the user recorded as its owner, by the file owner, or by an administrator. Possession of the token alone is not enough: the token is a publicly readable value (RFC 4918, section 6.5), so privileges are enforced through the normal permission mechanism as section 6.4 requires. + +### Enforcement + +Locks are enforced by file identity on every storage a file can be reached through: the owner's home storage, shares, group and team folders, external storages and custom mounts. A user lock blocks every other user, an app lock blocks everything outside the owning app's lock scope, and a token lock blocks everything that does not present the token as described above. Deleting or moving a folder is refused while it contains a file locked by someone else, so a locked file can neither be removed nor relocated through its parent. + ### Access paths An access path is the mechanism used to create or remove a lock. Locks can be created and removed through the Web UI, the OCS API, native WebDAV `LOCK`/`UNLOCK`, or the `X-User-Lock` WebDAV extension. These are access paths to the same lock implementation; they are not separate lock types. @@ -48,7 +54,7 @@ An access path is the mechanism used to create or remove a lock. Locks can be cr Native WebDAV locking is the token-based access path for Type 2 locks. - A `LOCK` request creates a token-owned lock and associates it with a WebDAV lock token. A subsequent `UNLOCK` request must provide that token. + A `LOCK` request creates a token-owned lock for the authenticated user and associates it with a WebDAV lock token. A subsequent `UNLOCK` request must provide that token and must come from a user who may write the file. A `Timeout` header sets the lifetime of the lock (`Second-N` or `Infinite`); without it the configured `lock_timeout` applies. A `LOCK` refresh (no body, the token in the `If` header) moves the expiry. The `` element of the request is not stored; the lock reports the display name of the user who created it. A `LOCK` on a file that already carries another lock answers `423 Locked`, an `UNLOCK` with an unknown token answers `409 Conflict`, and an `UNLOCK` the caller is not allowed to perform answers `403 Forbidden`. - **WebDAV with `X-User-Lock`** @@ -68,16 +74,18 @@ An access path is the mechanism used to create or remove a lock. Locks can be cr ### Unlocking -The file owner can override existing locks through access paths that support user override. Administrators can also force-unlock files using `occ files:lock --unlock `. In the case of automatic locks, apps and client applications are typically responsible for removing no longer needed locks. +The file owner can override existing locks on files stored in their own home storage (directly or through a share of it); on group folders, external storages and other mounts every user is reported as owner, so no owner override applies there. The user recorded as the owner of a lock can always release it, whatever the lock type. Administrators can force-unlock files using `occ files:lock --unlock `. In the case of automatic locks, apps and client applications are typically responsible for removing no longer needed locks. By default, files are locked indefinitely. -When unlocking an app-owned lock, provide a user ID that has access to the file: +A forced unlock removes the lock of any type and does not require the user who created it to still have access to the file: -`occ files:lock --unlock ` +`occ files:lock --unlock ` This command can be helpful when locks become stale. For example, when a user forgets to remove a manually created lock or a desktop client remains offline and automatic unlocking is not configured. +Deleting a file removes its lock. A file restored from the trash bin starts unlocked. + ### Lock timeout Locks have no expiry by default (`lock_timeout = -1`). @@ -88,12 +96,16 @@ Administrators can change the time of the maximum lock time in minutes (30) usin Set `lock_timeout` to `-1` to disable expiration. +Every lock carries an absolute expiry. Locking a file again (OCS, `X-User-Lock`, or a native WebDAV refresh) moves the expiry to the configured timeout, or to the timeout requested through the WebDAV `Timeout` header, counted from the moment of the refresh. Expired locks stop blocking immediately and a background job removes them from the database. + ### Locking Administrators can manually lock files using `occ`: `occ files:lock [] [--status] [--unlock]` +Only files can be locked through the Web UI, the OCS API, `X-User-Lock` and the command line; a folder ID is refused there. Native WebDAV clients may still lock a collection as RFC 4918 requires; such a lock protects the collection's own name and location, not its members. + ## Capabilities API If locking is available the app will expose itself through the capabilities endpoint under the `files` key: @@ -128,7 +140,7 @@ WebDAV returns the following additional properties in response to a `PROPFIND` r - `2` represents a token-owned lock, typically created through native WebDAV locking - `{http://nextcloud.org/ns}lock-owner`: User ID of the lock owner for user-owned and token-owned locks. This property is empty for app-owned locks. - `{http://nextcloud.org/ns}lock-owner-displayname`: Display name of the lock owner -- `{http://nextcloud.org/ns}lock-owner-editor`: App ID for an app-owned lock. Clients can use it to suggest joining the collaborative editing session in the web interface or through direct editing. In the response to an `X-User-Lock` `LOCK` request, this property currently contains the lock owner regardless of lock type. +- `{http://nextcloud.org/ns}lock-owner-editor`: App ID for an app-owned lock. Clients can use it to suggest joining the collaborative editing session in the web interface or through direct editing. It is empty for user-owned and token-owned locks on every access path. - `{http://nextcloud.org/ns}lock-time`: Timestamp at which the lock was created - `{http://nextcloud.org/ns}lock-timeout`: Lifetime of the lock in seconds counted from `lock-time`; it grows when the lock is refreshed. A value of `0` indicates that the lock does not expire. - `{http://nextcloud.org/ns}lock-token`: Lock token. Clients using native WebDAV locking must retain it while holding the lock and provide it when unlocking. @@ -185,6 +197,9 @@ The response will give back the updated properties after obtaining the lock with #### Error status codes +- 400 Unsupported `X-User-Lock-Type` +- 403 The caller lacks update permission, or the resource is a folder +- 404 The resource does not exist - 423 Unable to lock because the file is already locked by another owner ### Manually unlock a file @@ -221,8 +236,10 @@ curl -X UNLOCK \ #### Error status codes +- 400 Unsupported `X-User-Lock-Type` +- 404 The resource does not exist - 412 Unable to unlock because the file is not locked -- 423 Unable to unlock if the lock is owned by another user +- 423 Unable to unlock if the lock is owned by another user; the response carries the existing lock ## OCS API @@ -254,22 +271,37 @@ curl -X PUT 'http://admin:admin@nextcloud.local/ocs/v2.php/apps/files_lock/lock/ ``` #### Failure + +The file is already locked by someone else. The response carries the existing lock, without its token: ``` failure - 500 - + 423 + File is currently locked by admin - -1 - OCA\FilesLock\Exceptions\AlreadyLockedException - File is already locked by admin + 12 + admin + admin + 123 + -1 + 1648046707 + + 0 ``` +#### Status codes + +- 200 Lock created or refreshed +- 400 Unsupported `lockType`, invalid file ID, or the ID belongs to a folder +- 403 The caller lacks update permission on the file +- 404 The file does not exist or is not accessible to the caller +- 423 The file is locked by another owner + ### Unlocking a file `DELETE /apps/files_lock/lock/{fileId}` @@ -280,7 +312,7 @@ curl -X DELETE 'http://admin:admin@nextcloud.local/ocs/v2.php/apps/files_lock/lo #### Parameters -Not applicable. +- `lockType` (optional): The lock type the caller asserts, matching the value used when locking. The user recorded as the owner of a lock can release it whatever the type, and the file owner can release any lock on a file of their home storage. #### Success ``` @@ -295,18 +327,24 @@ Not applicable. ``` #### Failure + +The file is not locked: ``` failure - 500 - + 412 + File is not locked - - -1 - OCA\FilesLock\Exceptions\LockNotFoundException - - + ``` + +#### Status codes + +- 200 Lock released +- 400 Unsupported `lockType` or invalid file ID +- 404 The file does not exist or is not accessible to the caller +- 412 The file is not locked +- 423 The lock is held by someone else and the caller may not release it; the response carries the existing lock diff --git a/lib/Command/Lock.php b/lib/Command/Lock.php index 569a64c1..1862bb2b 100644 --- a/lib/Command/Lock.php +++ b/lib/Command/Lock.php @@ -26,6 +26,7 @@ use OCP\Files\InvalidPathException; use OCP\Files\Lock\ILock; use OCP\Files\Lock\LockContext; +use OCP\Files\Lock\OwnerLockedException; use OCP\Files\NotFoundException; use OCP\IUserManager; use OCP\User\Exceptions\UserNotFoundException; @@ -42,11 +43,10 @@ occ files:lock --status <file_id> Forcibly unlock a file: - occ files:lock --unlock <file_id> [<user_id>] + occ files:lock --unlock <file_id> -For app-owned locks, provide a user ID that has access to the file. This can be -needed for files stored in Groupfolders: - occ files:lock --unlock <file_id> <user_id> +A forced unlock removes the lock of any type regardless of who holds it and +does not require the lock owner to still have access to the file. Uninstall the app and delete all locks: occ files:lock --uninstall @@ -63,9 +63,6 @@ public function __construct( } /** - * @throws NotFoundException - * @throws UnauthorizedUnlockException - * @throws NotFileException * @throws InvalidPathException */ public function __invoke( @@ -73,7 +70,7 @@ public function __invoke( IInput $input, #[Argument(description: 'ID of the file to lock, unlock, or inspect', name: 'file_id')] ?string $fileId = null, - #[Argument(description: 'Lock owner when locking; user with file access when unlocking an app-owned lock', name: 'user_id')] + #[Argument(description: 'Lock owner when locking', name: 'user_id')] ?string $userId = null, #[Option(description: 'Fully uninstall the app from your Nextcloud')] bool $uninstall = false, @@ -98,14 +95,26 @@ public function __invoke( } if ($unlock === true) { - return $this->unlockFile($output, $userId, $fileId); + return $this->unlockFile($output, $fileId); } if ($userId === null || $userId === '') { throw new InvalidArgumentException('Not enough arguments (missing: "user_id")'); } - return $this->lockFile($output, $fileId, $userId); + try { + return $this->lockFile($output, $fileId, $userId); + } catch (OwnerLockedException $e) { + $output->writeln('File #' . $fileId . ' is already locked by ' . $e->getLock()->getOwner() . ''); + } catch (NotFileException) { + $output->writeln('#' . $fileId . ' is not a file; only files can be locked'); + } catch (UnauthorizedUnlockException|UserNotFoundException $e) { + $output->writeln('' . $e->getMessage() . ''); + } catch (NotFoundException) { + $output->writeln('File #' . $fileId . ' not found for user ' . $userId . ''); + } + + return ExitCode::Failure; } private function getStatus(IOutput $output, int $fileId): ExitCode { @@ -144,18 +153,15 @@ private function lockFile(IOutput $output, int $fileId, string $userId): ExitCod $file = $this->fileService->getFileFromId($user->getUID(), $fileId); $output->writeln('locking ' . $file->getName() . ' to ' . $userId . ''); - $this->lockService->lock(new LockContext( + $this->lockService->acquire(new LockContext( $file, ILock::TYPE_USER, $userId )); return ExitCode::Success; } - /** - * @throws UnauthorizedUnlockException - */ - private function unlockFile(IOutput $output, ?string $userId, int $fileId): ExitCode { + private function unlockFile(IOutput $output, int $fileId): ExitCode { try { - $this->lockService->unlockFile($fileId, $userId, true); + $this->lockService->forceUnlock($fileId); $output->writeln('Unlocked file #' . $fileId . ''); } catch (LockNotFoundException) { $output->writeln('File #' . $fileId . ' was already unlocked'); diff --git a/lib/Controller/LockController.php b/lib/Controller/LockController.php index c07d159e..0edddc2c 100644 --- a/lib/Controller/LockController.php +++ b/lib/Controller/LockController.php @@ -14,6 +14,7 @@ use OC\AppFramework\OCS\V2Response; use OCA\FilesLock\AppInfo\Application; use OCA\FilesLock\Exceptions\LockNotFoundException; +use OCA\FilesLock\Exceptions\NotFileException; use OCA\FilesLock\Exceptions\UnauthorizedUnlockException; use OCA\FilesLock\Model\FileLock; use OCA\FilesLock\Service\FileService; @@ -26,6 +27,8 @@ use OCP\Files\Lock\ILock; use OCP\Files\Lock\LockContext; use OCP\Files\Lock\OwnerLockedException; +use OCP\Files\NotFoundException; +use OCP\Files\NotPermittedException; use OCP\IL10N; use OCP\IRequest; use OCP\IUserSession; @@ -37,6 +40,7 @@ * @package OCA\FilesLock\Controller */ class LockController extends OCSController { + private const array SUPPORTED_LOCK_TYPES = [ILock::TYPE_USER, ILock::TYPE_APP, ILock::TYPE_TOKEN]; private int $ocsVersion; @@ -66,6 +70,13 @@ public function __construct( #[NoAdminRequired] #[NoSubAdminRequired] public function locking(string $fileId, int $lockType = ILock::TYPE_USER): DataResponse { + if (!in_array($lockType, self::SUPPORTED_LOCK_TYPES, true)) { + return $this->fail(new \InvalidArgumentException('Unsupported lock type'), [], Http::STATUS_BAD_REQUEST, false); + } + if (!is_numeric($fileId)) { + return $this->fail(new \InvalidArgumentException('Invalid file id'), [], Http::STATUS_BAD_REQUEST, false); + } + try { $user = $this->userSession->getUser(); if ($user === null) { @@ -73,13 +84,19 @@ public function locking(string $fileId, int $lockType = ILock::TYPE_USER): DataR } $file = $this->fileService->getFileFromId($user->getUID(), (int)$fileId); - $lock = $this->lockService->lock(new LockContext( + $lock = $this->lockService->acquire(new LockContext( $file, $lockType, $user->getUID() )); return new DataResponse($lock, Http::STATUS_OK); } catch (OwnerLockedException $e) { return new DataResponse($e->getLock(), Http::STATUS_LOCKED); + } catch (NotFoundException $e) { + return $this->fail($e, [], Http::STATUS_NOT_FOUND, false); + } catch (NotFileException $e) { + return $this->fail($e, [], Http::STATUS_BAD_REQUEST, false); + } catch (UnauthorizedUnlockException|NotPermittedException $e) { + return $this->fail($e, [], Http::STATUS_FORBIDDEN, false); } catch (Exception $e) { return $this->fail($e); } @@ -88,14 +105,20 @@ public function locking(string $fileId, int $lockType = ILock::TYPE_USER): DataR #[NoAdminRequired] #[NoSubAdminRequired] public function unlocking(string $fileId, int $lockType = ILock::TYPE_USER): DataResponse { + if (!in_array($lockType, self::SUPPORTED_LOCK_TYPES, true)) { + return $this->fail(new \InvalidArgumentException('Unsupported lock type'), [], Http::STATUS_BAD_REQUEST, false); + } + if (!is_numeric($fileId)) { + return $this->fail(new \InvalidArgumentException('Invalid file id'), [], Http::STATUS_BAD_REQUEST, false); + } + try { $user = $this->userSession->getUser(); if ($user === null) { throw new \LogicException('User not logged in'); } - $this->lockService->enableUserOverride(); - $this->lockService->unlockFile((int)$fileId, $user->getUID()); + $this->lockService->unlockFile((int)$fileId, $user->getUID(), false, $lockType); return new DataResponse(); } catch (LockNotFoundException) { @@ -103,14 +126,15 @@ public function unlocking(string $fileId, int $lockType = ILock::TYPE_USER): Dat $response->setStatus(Http::STATUS_PRECONDITION_FAILED); return $response; } catch (UnauthorizedUnlockException) { - try { - $lock = $this->lockService->getLockFromFileId((int)$fileId); - } catch (LockNotFoundException) { + $lock = $this->lockService->getActiveLock((int)$fileId); + if ($lock === null) { $response = new DataResponse(); $response->setStatus(Http::STATUS_PRECONDITION_FAILED); return $response; } return new DataResponse($lock, Http::STATUS_LOCKED); + } catch (NotFoundException $e) { + return $this->fail($e, [], Http::STATUS_NOT_FOUND, false); } catch (Exception $e) { return $this->fail($e); } @@ -133,7 +157,13 @@ private function buildOCSResponse(string $format, DataResponse $data): V1Respons } if ($containedData instanceof FileLock) { - $data->setData($containedData->jsonSerialize()); + $payload = $containedData->jsonSerialize(); + if ($data->getStatus() === Http::STATUS_LOCKED) { + // the token is the credential of a token lock and OCS never accepts + // one, so the caller that just lost the conflict has no use for it + unset($payload['token']); + } + $data->setData($payload); } if ($this->ocsVersion === 1) { diff --git a/tests/Feature/CommandTest.php b/tests/Feature/CommandTest.php index 696e030c..704c7844 100644 --- a/tests/Feature/CommandTest.php +++ b/tests/Feature/CommandTest.php @@ -13,12 +13,14 @@ use OCA\FilesLock\Command\Lock; use OCP\Files\Lock\ILock; use OCP\Files\Lock\LockContext; +use OCP\Share\IManager as IShareManager; +use OCP\Share\IShare; use PHPUnit\Framework\Attributes\Group; use Psr\Container\ContainerInterface; use Symfony\Component\Console\Tester\CommandTester; /** - * occ files:lock: status, locking and unlocking. + * occ files:lock: status, locking, and forced unlock as an administrative operation. */ #[Group(name: 'DB')] class CommandTest extends LockTestCase { @@ -43,11 +45,60 @@ public function testStatusAndLock(): void { self::assertStringContainsString('locked by ' . self::USER1, $tester->getDisplay()); } - public function testUnlock(): void { - $file = $this->loginAndGetUserFolder(self::USER1)->newFile('cli-unlock.txt', 'AAA'); + public function testLockingAnAlreadyLockedFileFailsCleanly(): void { + $file = $this->sharedFile('cli-conflict.txt'); $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())); + $tester = $this->tester(); + self::assertSame(1, $tester->execute(['file_id' => (string)$file->getId(), 'user_id' => self::USER2])); + self::assertStringContainsString('already locked by ' . self::USER1, $tester->getDisplay()); + self::assertSame(1, $this->lockRowCount($file->getId()), 'the existing lock is untouched'); + } + + public function testLockingAFolderFailsCleanly(): void { + $folder = $this->loginAndGetUserFolder(self::USER1)->newFolder('cli-folder'); + + $tester = $this->tester(); + self::assertSame(1, $tester->execute(['file_id' => (string)$folder->getId(), 'user_id' => self::USER1])); + self::assertStringContainsString('not a file', $tester->getDisplay()); + self::assertSame(0, $this->lockRowCount($folder->getId())); + } + + public function testForcedUnlockAfterOwnerLostAccess(): void { + $file = $this->sharedFile('cli-force.txt'); + $id = $file->getId(); + $shared = $this->loginAndGetUserFolder(self::USER2)->get('cli-force.txt'); + $this->lockManager->lock(new LockContext($shared, ILock::TYPE_USER, self::USER2)); + + $shareManager = \OCP\Server::get(IShareManager::class); + foreach ($shareManager->getSharesBy(self::USER1, IShare::TYPE_USER, $file) as $share) { + $shareManager->deleteShare($share); + } + $this->logout(); + $tester = $this->tester(); + + self::assertSame(0, $tester->execute(['file_id' => (string)$id, '--unlock' => true])); + self::assertStringContainsString('Unlocked file #' . $id, $tester->getDisplay()); + self::assertSame(0, $this->lockRowCount($id)); + + self::assertSame(0, $tester->execute(['file_id' => (string)$id, '--unlock' => true])); + self::assertStringContainsString('already unlocked', $tester->getDisplay()); + } + + public function testForcedUnlockOfAppAndTokenLocks(): void { + $file = $this->loginAndGetUserFolder(self::USER1)->newFile('cli-app.txt', 'AAA'); + $id = $file->getId(); + $tester = $this->tester(); + + $this->lockManager->lock(new LockContext($file, ILock::TYPE_APP, 'text')); + $this->logout(); + self::assertSame(0, $tester->execute(['file_id' => (string)$id, '--unlock' => true])); + self::assertSame(0, $this->lockRowCount($id)); + + $this->loginAndGetUserFolder(self::USER1); + $this->lockService()->acquire(new LockContext($file, ILock::TYPE_TOKEN, self::USER1), null, 'cli-token'); + $this->logout(); + self::assertSame(0, $tester->execute(['file_id' => (string)$id, '--unlock' => true])); + self::assertSame(0, $this->lockRowCount($id)); } } diff --git a/tests/Feature/OcsControllerTest.php b/tests/Feature/OcsControllerTest.php index b112ecfe..cb2d82a6 100644 --- a/tests/Feature/OcsControllerTest.php +++ b/tests/Feature/OcsControllerTest.php @@ -48,6 +48,10 @@ public function testLockUnlockRoundTrip(): void { 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']); + self::assertSame(-1, $body['ocs']['data']['eta']); + + [$status, $body] = $this->render($this->controller()->locking((string)$file->getId())); + self::assertSame(Http::STATUS_OK, $status, 'locking again refreshes'); [$status] = $this->render($this->controller()->unlocking((string)$file->getId())); self::assertSame(Http::STATUS_OK, $status); @@ -70,15 +74,70 @@ public function testConflictIsAStructured423(): void { self::assertSame(self::USER1, $body['ocs']['data']['userId']); self::assertSame($file->getId(), $body['ocs']['data']['fileId']); self::assertStringContainsString('locked by', $body['ocs']['meta']['message']); + self::assertArrayNotHasKey('token', $body['ocs']['data'], 'the conflict payload must not hand out the lock token'); } else { self::assertStringContainsString('' . self::USER1 . '', $body); self::assertStringContainsString('423', $body); + self::assertStringNotContainsString('', $body, 'the conflict payload must not hand out the lock token'); } } [$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::assertArrayNotHasKey('token', $body['ocs']['data'], 'nor may the failed unlock hand it out'); self::assertSame(1, $this->lockRowCount($file->getId())); } + + public function testFileOwnerReleasesRecipientLock(): void { + $file = $this->sharedFile('override.txt'); + $this->loginAndGetUserFolder(self::USER2); + [$status] = $this->render($this->controller()->locking((string)$file->getId())); + self::assertSame(Http::STATUS_OK, $status); + + $this->loginAndGetUserFolder(self::USER1); + [$status] = $this->render($this->controller()->unlocking((string)$file->getId())); + self::assertSame(Http::STATUS_OK, $status); + self::assertSame(0, $this->lockRowCount($file->getId())); + } + + public function testLockTypeIsHonouredOnUnlock(): void { + $file = $this->sharedFile('typed.txt'); + $this->loginAndGetUserFolder(self::USER2); + [$status, $body] = $this->render($this->controller()->locking((string)$file->getId(), ILock::TYPE_APP)); + self::assertSame(Http::STATUS_OK, $status); + self::assertSame(ILock::TYPE_APP, $body['ocs']['data']['type']); + self::assertSame(self::USER2, $body['ocs']['data']['userId'], 'the session user is the recorded owner'); + + [$status] = $this->render($this->controller()->unlocking((string)$file->getId(), ILock::TYPE_APP)); + self::assertSame(Http::STATUS_OK, $status, 'the recorded owner releases through the same path'); + + [$status, $body] = $this->render($this->controller()->locking((string)$file->getId(), ILock::TYPE_TOKEN)); + self::assertSame(Http::STATUS_OK, $status); + self::assertSame(ILock::TYPE_TOKEN, $body['ocs']['data']['type']); + [$status] = $this->render($this->controller()->unlocking((string)$file->getId())); + self::assertSame(Http::STATUS_OK, $status, 'the recorded owner releases a token lock without the token'); + } + + public function testClientErrors(): void { + $folder = $this->loginAndGetUserFolder(self::USER1)->newFolder('folder'); + [$status] = $this->render($this->controller()->locking('999999999')); + self::assertSame(Http::STATUS_NOT_FOUND, $status); + [$status] = $this->render($this->controller()->unlocking('999999999')); + self::assertSame(Http::STATUS_NOT_FOUND, $status); + [$status] = $this->render($this->controller()->locking((string)$folder->getId())); + self::assertSame(Http::STATUS_BAD_REQUEST, $status); + [$status] = $this->render($this->controller()->locking((string)$folder->getId(), 99)); + self::assertSame(Http::STATUS_BAD_REQUEST, $status); + [$status] = $this->render($this->controller()->locking((string)$folder->getId(), -1)); + self::assertSame(Http::STATUS_BAD_REQUEST, $status); + [$status] = $this->render($this->controller()->locking('abc')); + self::assertSame(Http::STATUS_BAD_REQUEST, $status); + + $file = $this->sharedFile('readonly.txt', 1); + $this->loginAndGetUserFolder(self::USER2); + [$status, $body] = $this->render($this->controller()->locking((string)$file->getId())); + self::assertSame(Http::STATUS_FORBIDDEN, $status); + self::assertSame(0, $this->lockRowCount($file->getId())); + } } From aec24721b0e14f20896c8c5e4708f1c07a02b217 Mon Sep 17 00:00:00 2001 From: Git'Fellow <12234510+solracsf@users.noreply.github.com> Date: Mon, 7 Sep 2026 15:51:47 +0200 Subject: [PATCH 6/8] test: cover locking under collaborative multi-client churn The suite tested one actor at a time, so nothing covered what a real deployment does to a lock: several people editing, renaming, moving and deleting the same tree from the browser, the sync client and the mobile apps at once. A lock has to be placed and released exactly where it is needed, and everything else has to stay out of the way. Native WebDAV, through a real Sabre server: - a manually locked file keeps its lock when its holder renames it, is reported at the new path and still refuses the other user there - only the client holding a token lock moves the file: the same user's browser is refused, another user is refused even with the published token, and everyone is free again the moment UNLOCK arrives - a client that never sends UNLOCK stops holding the file once the configured timeout passes, without waiting for the cleanup job - directory listings follow locks other clients take and release, which is where a stale hit in the bulk PROPFIND cache would show up as a phantom lock Storage level: - moving a file to another storage copies it under a new id and deletes the source, so the lock must neither outlive the old id nor follow onto a file nobody locked - a second user cannot move a held file out onto a mount of their own Green on MariaDB 11.8, PostgreSQL 16 and S3 primary storage. Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com> --- tests/Feature/DavLockTest.php | 85 +++++++++++++++++++++++++++++++++ tests/Feature/LifecycleTest.php | 46 ++++++++++++++++++ 2 files changed, 131 insertions(+) diff --git a/tests/Feature/DavLockTest.php b/tests/Feature/DavLockTest.php index 92edebfd..645f606e 100644 --- a/tests/Feature/DavLockTest.php +++ b/tests/Feature/DavLockTest.php @@ -369,6 +369,91 @@ public function testOtherUsersLockIsVisibleButNotUsable(): void { self::assertSame(423, $this->request(self::USER2, 'PROPPATCH', '/visible.txt', '1')->getStatus()); } + public function testALockedFileStaysLockedAfterItsHolderMovesIt(): void { + $dir = $this->loginAndGetUserFolder(self::USER1)->newFolder('churn'); + $file = $dir->newFile('travel.txt', 'AAA'); + $this->shareWith($dir, self::USER1, self::USER2, 31); + $id = $file->getId(); + $this->lockManager->lock(new LockContext($file, ILock::TYPE_USER, self::USER1)); + + self::assertContains($this->request(self::USER1, 'MOVE', '/churn/travel.txt', null, ['Destination' => '/churn/travelled.txt'])->getStatus(), [201, 204]); + self::assertSame(1, $this->lockRowCount($id)); + + $props = $this->lockProps(self::USER2, '/churn/travelled.txt'); + self::assertSame('1', $props['lock']); + self::assertSame(self::USER1, $props['lock-owner']); + self::assertSame(423, $this->request(self::USER2, 'PUT', '/churn/travelled.txt', 'BBB')->getStatus()); + } + + /** + * The web client never holds the lock token of a lock its own desktop client + * took, so it is refused like any other client until that client releases it. + */ + public function testOnlyTheClientHoldingATokenLockMovesTheFile(): void { + $dir = $this->loginAndGetUserFolder(self::USER1)->newFolder('churn'); + $file = $dir->newFile('doc.odt', 'AAA'); + $this->shareWith($dir, self::USER1, self::USER2, 31); + $id = $file->getId(); + $token = $this->tokenOf($this->nativeLock(self::USER1, '/churn/doc.odt')); + $if = ['If' => '()']; + + self::assertSame(423, $this->request(self::USER1, 'MOVE', '/churn/doc.odt', null, ['Destination' => '/churn/renamed.odt'])->getStatus()); + + self::assertContains($this->request(self::USER1, 'MOVE', '/churn/doc.odt', null, ['Destination' => '/churn/renamed.odt'] + $if)->getStatus(), [201, 204]); + self::assertSame(1, $this->lockRowCount($id)); + self::assertSame($token, $this->lockProps(self::USER2, '/churn/renamed.odt')['lock-token']); + + self::assertSame(204, $this->request(self::USER1, 'UNLOCK', '/churn/renamed.odt', null, ['Lock-Token' => ''])->getStatus()); + self::assertContains($this->request(self::USER2, 'MOVE', '/churn/renamed.odt', null, ['Destination' => '/churn/theirs.odt'])->getStatus(), [201, 204]); + } + + /** + * A client that never sends UNLOCK (crashed editor, killed sync client) must + * not hold the file for the rest of the team beyond the configured timeout. + */ + public function testAnAbandonedClientLockStopsBlockingWhenItExpires(): void { + $this->setLockTimeoutMinutes(30); + $this->toTheFuture(0); + $dir = $this->loginAndGetUserFolder(self::USER1)->newFolder('churn'); + $dir->newFile('abandoned.txt', 'AAA'); + $this->shareWith($dir, self::USER1, self::USER2, 31); + $this->nativeLock(self::USER1, '/churn/abandoned.txt'); + + self::assertSame(423, $this->request(self::USER2, 'MOVE', '/churn/abandoned.txt', null, ['Destination' => '/churn/mine.txt'])->getStatus()); + + $this->toTheFuture(1801); + self::assertSame('', $this->lockProps(self::USER2, '/churn/abandoned.txt')['lock']); + self::assertContains($this->request(self::USER2, 'MOVE', '/churn/abandoned.txt', null, ['Destination' => '/churn/mine.txt'])->getStatus(), [201, 204]); + } + + /** + * Locks other clients take and release between two listings, which is where a + * stale hit in the bulk PROPFIND cache would surface as a phantom lock. + */ + public function testDirectoryListingsFollowLocksTakenByOtherClients(): void { + $dir = $this->loginAndGetUserFolder(self::USER1)->newFolder('churn'); + $one = $dir->newFile('one.txt', 'AAA'); + $dir->newFile('two.txt', 'BBB'); + $this->shareWith($dir, self::USER1, self::USER2, 31); + + self::assertSame(0, $this->countLockOwners(self::USER2, '/churn/')); + + $lock = $this->lockManager->lock(new LockContext($one, ILock::TYPE_USER, self::USER1)); + self::assertSame(1, $this->countLockOwners(self::USER2, '/churn/')); + self::assertSame(423, $this->request(self::USER2, 'PUT', '/churn/one.txt', 'CCC')->getStatus()); + self::assertContains($this->request(self::USER2, 'PUT', '/churn/two.txt', 'CCC')->getStatus(), [200, 204]); + + self::assertSame(204, $this->request(self::USER1, 'UNLOCK', '/churn/one.txt', null, ['Lock-Token' => 'getToken() . '>'])->getStatus()); + self::assertSame(0, $this->countLockOwners(self::USER2, '/churn/')); + self::assertContains($this->request(self::USER2, 'PUT', '/churn/one.txt', 'DDD')->getStatus(), [200, 204]); + } + + private function countLockOwners(string $user, string $path): int { + $response = $this->request($user, 'PROPFIND', $path, '', ['Depth' => '1']); + self::assertSame(207, $response->getStatus()); + return preg_match_all('#[^<]+#', $this->body($response)); + } + #[\Override] protected function sharedFile(string $name, int $permissions = 19, ?int $permissionsUser3 = null): File { return parent::sharedFile($name, $permissions, $permissionsUser3); diff --git a/tests/Feature/LifecycleTest.php b/tests/Feature/LifecycleTest.php index db808737..f28d948b 100644 --- a/tests/Feature/LifecycleTest.php +++ b/tests/Feature/LifecycleTest.php @@ -9,6 +9,8 @@ namespace OCA\FilesLock\Tests\Feature; +use OC\Files\Filesystem; +use OC\Files\Storage\Temporary; use OCA\Files_Trashbin\Helper; use OCA\Files_Trashbin\Trashbin; use OCA\FilesLock\Db\LocksRequest; @@ -17,6 +19,7 @@ use OCP\Files\Events\Node\NodeDeletedEvent; use OCP\Files\Lock\ILock; use OCP\Files\Lock\LockContext; +use OCP\Lock\LockedException; use PHPUnit\Framework\Attributes\Group; /** @@ -99,6 +102,49 @@ public function testRestoredFileIsNotLocked(): void { self::assertSame(0, $this->lockRowCount($id)); } + /** + * Moving a file between storages copies it under a new id and deletes the + * source, so the lock must not survive as a row pointing at a file that is + * gone, and it must not follow onto a file nobody locked. + */ + public function testMovingALockedFileToAnotherStorageStrandsNoLock(): void { + $folder = $this->loginAndGetUserFolder(self::USER1); + $storage = new Temporary([]); + Filesystem::mount($storage, [], '/' . self::USER1 . '/files/ext/'); + $file = $folder->newFile('crossing.txt', 'AAA'); + $id = $file->getId(); + $this->lockManager->lock(new LockContext($file, ILock::TYPE_USER, self::USER1)); + + $file->move($folder->get('ext')->getPath() . '/crossing.txt'); + + $moved = $this->rootFolder->getUserFolder(self::USER1)->get('ext/crossing.txt'); + self::assertSame(0, $this->lockRowCount($id)); + self::assertSame(0, $this->lockRowCount($moved->getId())); + } + + public function testAnotherUserCannotMoveALockedFileOffItsStorage(): void { + $folder = $this->loginAndGetUserFolder(self::USER1); + $dir = $folder->newFolder('shared-tree'); + $file = $dir->newFile('held.txt', 'AAA'); + $this->shareWith($dir, self::USER1, self::USER2, 31); + $id = $file->getId(); + $this->lockManager->lock(new LockContext($file, ILock::TYPE_USER, self::USER1)); + + \OC_Util::setupFS(self::USER2); + \OC_User::setUserId(self::USER2); + $this->lockService()->clearCache(); + $storage = new Temporary([]); + Filesystem::mount($storage, [], '/' . self::USER2 . '/files/ext/'); + $theirs = $this->rootFolder->getUserFolder(self::USER2)->get('shared-tree/held.txt'); + + try { + $theirs->move($this->rootFolder->getUserFolder(self::USER2)->get('ext')->getPath() . '/held.txt'); + self::fail('a locked file should not leave its storage under another user'); + } catch (LockedException) { + } + self::assertSame(1, $this->lockRowCount($id)); + } + public function testPurgingTheCacheEntryRemovesTheLock(): void { $file = $this->loginAndGetUserFolder(self::USER1)->newFile('purged.txt', 'AAA'); $id = $file->getId(); From 106c90db9e72661b72223466092b0f9aca38a42f Mon Sep 17 00:00:00 2001 From: Git'Fellow <12234510+solracsf@users.noreply.github.com> Date: Wed, 9 Sep 2026 10:33:50 +0200 Subject: [PATCH 7/8] refactor: remove what the rewrite left behind and fix what a review found Rebuilding acquisition and authorization left a set of members that nothing reaches any more, plus a few shapes that only existed to serve the old code. - canUnlock(), update() and generateToken() on LockService lost their last callers when unlock() started asking the policy directly and acquire() started building the whole lock itself. enableUserOverride() had already been reduced to an empty stub, so anyone still calling it was quietly getting nothing rather than the override they asked for. - prefetchRemoteLocks() never performed a listing. It resolved the storage, checked whether it was a DAV one, and returned either way, so nothing warmed the remote properties. The per-directory batching that does work is getLockForNodeIds(), which reads the local locks of a whole listing in one query. Warming the remote properties is still worth doing, but it needs a remote to measure against and belongs in its own change. - getLockForNodeId() returns ?FileLock rather than FileLock|false, which is what forced all eight DAV property handlers to open with the same check for false. - gone as well: the getDeprecatedLocks() alias, the unused $current of canLock(), the timeout argument of fromLockScope() that every caller passed as 0, the expiresAt branches of import() that its one caller never passes, and an IEventDispatcher that LockService has been taking and dropping on the floor since before this branch. A read through the result then turned up a handful of defects in the new code, fixed here rather than left for a follow-up: - acquire() retried a token collision without covering the retry itself, so a competing insert landing in that window escaped as an unhandled LockConflictException; only the OCS path caught it by accident. The retry now reads the winner back and reports it as the conflict it is. - acquire() deleted an expired row before the select that says whether there is one, a wasted round trip on the common case of a file nobody has locked. It selects first and deletes only what it found, keeping the delete's own expiry condition so a refresh in between is still not dropped. - canLock() had its own copy of the permission check without the guard canModify() wraps it in, so a storage that throws while reporting permissions became a fatal on the DAV paths instead of a refusal. - getLockForNodeIds() left a file with no lock out of its result rather than reporting it as false, against what the signature promises. - rmdir() resolved the folder and ran the locks-below join twice. checkDescendants() returns what it read and rmdir() uses it, which also retires getBlockingLocksBelow(). - the source-lock check that copyFromStorage() and moveFromStorage() each carried verbatim is one method now, and the lock types both access paths accept come from one constant instead of two identical private ones. Folders staying unlockable through ILockManager is deliberate, but the README only listed the other four paths, so it says so now. Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com> --- README.md | 2 +- lib/AppInfo/Application.php | 3 + lib/Controller/LockController.php | 5 +- lib/Cron/Unlock.php | 4 - lib/DAV/LockBackend.php | 6 +- lib/DAV/LockPlugin.php | 113 ++++------- lib/Db/LocksRequest.php | 3 +- .../BeforeFileSystemSetupListener.php | 3 - lib/Model/FileLock.php | 15 +- lib/Service/LockService.php | 190 ++++++------------ lib/Storage/LockWrapper.php | 81 ++++---- tests/Feature/AcquisitionTest.php | 22 +- tests/Feature/LockFeatureTest.php | 8 +- 13 files changed, 181 insertions(+), 274 deletions(-) diff --git a/README.md b/README.md index cc0e803e..afe19e03 100644 --- a/README.md +++ b/README.md @@ -104,7 +104,7 @@ Administrators can manually lock files using `occ`: `occ files:lock [] [--status] [--unlock]` -Only files can be locked through the Web UI, the OCS API, `X-User-Lock` and the command line; a folder ID is refused there. Native WebDAV clients may still lock a collection as RFC 4918 requires; such a lock protects the collection's own name and location, not its members. +Only files can be locked through the Web UI, the OCS API, `X-User-Lock`, the command line and the PHP `ILockManager` API; a folder is refused there. Native WebDAV clients may still lock a collection as RFC 4918 requires; such a lock protects the collection's own name and location, not its members. ## Capabilities API diff --git a/lib/AppInfo/Application.php b/lib/AppInfo/Application.php index f7e79969..94b73211 100644 --- a/lib/AppInfo/Application.php +++ b/lib/AppInfo/Application.php @@ -25,11 +25,14 @@ use OCP\Files\Events\BeforeFileSystemSetupEvent; use OCP\Files\Events\BeforeRemotePropfindEvent; use OCP\Files\Events\Node\NodeDeletedEvent; +use OCP\Files\Lock\ILock; use OCP\Files\Lock\ILockManager; class Application extends App implements IBootstrap { public const APP_ID = 'files_lock'; + public const array SUPPORTED_LOCK_TYPES = [ILock::TYPE_USER, ILock::TYPE_APP, ILock::TYPE_TOKEN]; + public const DAV_PROPERTY_LOCK = '{http://nextcloud.org/ns}lock'; public const DAV_PROPERTY_LOCK_OWNER_TYPE = '{http://nextcloud.org/ns}lock-owner-type'; public const DAV_PROPERTY_LOCK_OWNER = '{http://nextcloud.org/ns}lock-owner'; diff --git a/lib/Controller/LockController.php b/lib/Controller/LockController.php index 0edddc2c..ca8e2944 100644 --- a/lib/Controller/LockController.php +++ b/lib/Controller/LockController.php @@ -40,7 +40,6 @@ * @package OCA\FilesLock\Controller */ class LockController extends OCSController { - private const array SUPPORTED_LOCK_TYPES = [ILock::TYPE_USER, ILock::TYPE_APP, ILock::TYPE_TOKEN]; private int $ocsVersion; @@ -70,7 +69,7 @@ public function __construct( #[NoAdminRequired] #[NoSubAdminRequired] public function locking(string $fileId, int $lockType = ILock::TYPE_USER): DataResponse { - if (!in_array($lockType, self::SUPPORTED_LOCK_TYPES, true)) { + if (!in_array($lockType, Application::SUPPORTED_LOCK_TYPES, true)) { return $this->fail(new \InvalidArgumentException('Unsupported lock type'), [], Http::STATUS_BAD_REQUEST, false); } if (!is_numeric($fileId)) { @@ -105,7 +104,7 @@ public function locking(string $fileId, int $lockType = ILock::TYPE_USER): DataR #[NoAdminRequired] #[NoSubAdminRequired] public function unlocking(string $fileId, int $lockType = ILock::TYPE_USER): DataResponse { - if (!in_array($lockType, self::SUPPORTED_LOCK_TYPES, true)) { + if (!in_array($lockType, Application::SUPPORTED_LOCK_TYPES, true)) { return $this->fail(new \InvalidArgumentException('Unsupported lock type'), [], Http::STATUS_BAD_REQUEST, false); } if (!is_numeric($fileId)) { diff --git a/lib/Cron/Unlock.php b/lib/Cron/Unlock.php index 812980f2..c2aaa848 100644 --- a/lib/Cron/Unlock.php +++ b/lib/Cron/Unlock.php @@ -25,10 +25,6 @@ public function __construct( #[\Override] protected function run($argument): void { - $this->deleteExpiredLocks(); - } - - private function deleteExpiredLocks(): void { $this->lockService->removeLocksIfExpired($this->lockService->getExpiredLocks(1000)); } } diff --git a/lib/DAV/LockBackend.php b/lib/DAV/LockBackend.php index a197ff06..c5afeec0 100644 --- a/lib/DAV/LockBackend.php +++ b/lib/DAV/LockBackend.php @@ -101,10 +101,8 @@ public function lock($uri, LockInfo $lockInfo): bool { $timeout = null; if ($lockInfo->timeout !== null) { - $timeout = $lockInfo->timeout === LockInfo::TIMEOUT_INFINITE ? FileLock::ETA_INFINITE : max(0, (int)$lockInfo->timeout); - if ($timeout === 0) { - $timeout = FileLock::ETA_INFINITE; - } + $seconds = (int)$lockInfo->timeout; + $timeout = $seconds > 0 ? $seconds : FileLock::ETA_INFINITE; } try { diff --git a/lib/DAV/LockPlugin.php b/lib/DAV/LockPlugin.php index 3b6f41d3..f33cf63e 100644 --- a/lib/DAV/LockPlugin.php +++ b/lib/DAV/LockPlugin.php @@ -35,7 +35,7 @@ use Sabre\HTTP\ResponseInterface; class LockPlugin extends SabreLockPlugin { - private const array SUPPORTED_LOCK_TYPES = [ILock::TYPE_USER, ILock::TYPE_APP, ILock::TYPE_TOKEN]; + private const string TOKEN_PREFIX = 'opaquelocktoken:'; public function __construct( private readonly LockService $lockService, @@ -95,7 +95,6 @@ private function cacheDirectory(Directory $directory): void { $ids[] = (int)$directory->getId(); // the lock service will take care of the caching $this->lockService->getLockForNodeIds($ids); - $this->lockService->prefetchRemoteLocks($directory->getNode()); } public function customProperties(PropFind $propFind, INode $node): void { @@ -112,84 +111,48 @@ public function customProperties(PropFind $propFind, INode $node): void { $nodeId = $node->getId(); - $propFind->handle(Application::DAV_PROPERTY_LOCK, function () use ($nodeId, $node): bool { - $lock = $this->lockService->getLockForNodeId($nodeId, $node->getNode()); - return $lock instanceof FileLock; - }); - - $propFind->handle(Application::DAV_PROPERTY_LOCK_OWNER, function () use ($nodeId, $node): ?string { - $lock = $this->lockService->getLockForNodeId($nodeId, $node->getNode()); + $lockOf = fn (): ?FileLock => $this->lockService->getLockForNodeId($nodeId, $node->getNode()); - if ($lock === false) { - return null; - } - - if ($lock->getType() === ILock::TYPE_APP) { - return null; - } + $propFind->handle(Application::DAV_PROPERTY_LOCK, fn (): bool => $lockOf() !== null); - return $lock->getOwner(); + $propFind->handle(Application::DAV_PROPERTY_LOCK_OWNER, function () use ($lockOf): ?string { + $lock = $lockOf(); + return $lock === null || $lock->getType() === ILock::TYPE_APP ? null : $lock->getOwner(); }); - $propFind->handle(Application::DAV_PROPERTY_LOCK_TIME, function () use ($nodeId, $node): ?int { - $lock = $this->lockService->getLockForNodeId($nodeId, $node->getNode()); + $propFind->handle(Application::DAV_PROPERTY_LOCK_TIME, fn (): ?int => $lockOf()?->getCreatedAt()); - if ($lock === false) { - return null; - } - - return $lock->getCreatedAt(); + $propFind->handle(Application::DAV_PROPERTY_LOCK_TIMEOUT, function () use ($lockOf): ?int { + $lock = $lockOf(); + return $lock === null ? null : $this->davTimeout($lock); }); - $propFind->handle(Application::DAV_PROPERTY_LOCK_TIMEOUT, function () use ($nodeId, $node): ?int { - $lock = $this->lockService->getLockForNodeId($nodeId, $node->getNode()); - - if ($lock === false) { - return null; - } - - return $this->davTimeout($lock); + $propFind->handle(Application::DAV_PROPERTY_LOCK_OWNER_DISPLAYNAME, function () use ($lockOf): ?string { + $lock = $lockOf(); + return $lock === null ? null : $this->lockService->injectMetadata($lock)->getDisplayName(); }); - $propFind->handle(Application::DAV_PROPERTY_LOCK_OWNER_DISPLAYNAME, function () use ($nodeId, $node): ?string { - $lock = $this->lockService->getLockForNodeId($nodeId, $node->getNode()); - - if ($lock === false) { - return null; - } - - $this->lockService->injectMetadata($lock); + $propFind->handle(Application::DAV_PROPERTY_LOCK_OWNER_TYPE, fn (): ?int => $lockOf()?->getType()); - return $lock->getDisplayName(); + $propFind->handle(Application::DAV_PROPERTY_LOCK_EDITOR, function () use ($lockOf): ?string { + $lock = $lockOf(); + return $lock?->getType() === ILock::TYPE_APP ? $lock->getOwner() : null; }); - $propFind->handle(Application::DAV_PROPERTY_LOCK_OWNER_TYPE, function () use ($nodeId, $node): ?int { - $lock = $this->lockService->getLockForNodeId($nodeId, $node->getNode()); - - if ($lock === false) { - return null; - } - - return $lock->getType(); - }); - - $propFind->handle(Application::DAV_PROPERTY_LOCK_EDITOR, function () use ($nodeId, $node): ?string { - $lock = $this->lockService->getLockForNodeId($nodeId, $node->getNode()); - if ($lock === false || $lock->getType() !== ILock::TYPE_APP) { - return null; - } - - return $lock->getOwner(); - }); - - $propFind->handle(Application::DAV_PROPERTY_LOCK_TOKEN, function () use ($nodeId, $node): ?string { - $lock = $this->lockService->getLockForNodeId($nodeId, $node->getNode()); - if ($lock === false) { - return null; - } + $propFind->handle(Application::DAV_PROPERTY_LOCK_TOKEN, fn (): ?string => $lockOf()?->getToken()); + } - return $lock->getToken(); - }); + /** + * The lock token carried by one entry of an If header, or null when it is not + * one of ours. + * + * @param array{token: mixed} $token + */ + private static function lockToken(array $token): ?string { + $value = (string)$token['token']; + return str_starts_with($value, self::TOKEN_PREFIX) + ? substr($value, strlen(self::TOKEN_PREFIX)) + : null; } /** @@ -204,8 +167,9 @@ public function validateTokens(RequestInterface $request, &$conditions): void { $this->lockService->resetPresentedTokens(); foreach ($conditions as $condition) { foreach ($condition['tokens'] as $token) { - if (str_starts_with((string)$token['token'], 'opaquelocktoken:')) { - $this->lockService->presentToken(substr((string)$token['token'], 16)); + $presented = self::lockToken($token); + if ($presented !== null) { + $this->lockService->presentToken($presented); } } } @@ -248,10 +212,10 @@ public function validateTokens(RequestInterface $request, &$conditions): void { foreach ($conditions as $kk => $condition) { foreach ($condition['tokens'] as $ii => $token) { - if (!str_starts_with((string)$token['token'], 'opaquelocktoken:')) { + $checkToken = self::lockToken($token); + if ($checkToken === null) { continue; } - $checkToken = substr((string)$token['token'], 16); if (isset($byToken[$checkToken])) { $conditions[$kk]['tokens'][$ii]['validToken'] = true; continue; @@ -284,11 +248,6 @@ public function httpLock(RequestInterface $request, ResponseInterface $response) throw new Forbidden('Locking requires an authenticated user'); } - $user = $this->userSession->getUser(); - if ($user === null) { - throw new \LogicException('User not logged in'); - } - try { $lockInfo = $this->lockService->acquire(new LockContext( $file, $lockType, $user->getUID() @@ -372,7 +331,7 @@ private function getRequestedLockType(RequestInterface $request): int { if ($header === null || $header === '') { return ILock::TYPE_USER; } - if (!is_numeric($header) || !in_array((int)$header, self::SUPPORTED_LOCK_TYPES, true)) { + if (!is_numeric($header) || !in_array((int)$header, Application::SUPPORTED_LOCK_TYPES, true)) { throw new \Sabre\DAV\Exception\BadRequest('Unsupported lock type'); } return (int)$header; diff --git a/lib/Db/LocksRequest.php b/lib/Db/LocksRequest.php index 8c35e43b..cc4b1818 100644 --- a/lib/Db/LocksRequest.php +++ b/lib/Db/LocksRequest.php @@ -230,7 +230,8 @@ public function getLocksBelow(int $folderId): array { $prefix = ($folder['path'] === null || $folder['path'] === '') ? '' : $folder['path'] . '/'; $qb = $this->connection->getQueryBuilder(); - $qb->select('l.id', 'l.user_id', 'l.file_id', 'l.token', 'l.creation', 'l.type', 'l.ttl', 'l.owner', 'l.scope', 'l.expires_at', 'f.path') + $qb->select(...array_map(static fn (string $column): string => 'l.' . $column, self::COLUMNS)) + ->addSelect('f.path') ->from(self::TABLE_LOCKS, 'l') ->innerJoin('l', 'filecache', 'f', $qb->expr()->eq('l.file_id', 'f.fileid')) ->where($qb->expr()->eq('f.storage', $qb->createNamedParameter((int)$folder['storage'], IQueryBuilder::PARAM_INT))); diff --git a/lib/Listeners/BeforeFileSystemSetupListener.php b/lib/Listeners/BeforeFileSystemSetupListener.php index a0a577ec..3032bf73 100644 --- a/lib/Listeners/BeforeFileSystemSetupListener.php +++ b/lib/Listeners/BeforeFileSystemSetupListener.php @@ -17,7 +17,6 @@ use OCP\Files\Events\BeforeFileSystemSetupEvent; use OCP\Files\Lock\ILockManager; use OCP\Files\Storage\IStorage; -use OCP\IUserSession; use Override; /** @@ -26,7 +25,6 @@ class BeforeFileSystemSetupListener implements IEventListener { public function __construct( private readonly ILockManager $lockManager, - private readonly IUserSession $userSession, private readonly LockService $lockService, ) { } @@ -45,7 +43,6 @@ public function handle(Event $event): void { [ 'storage' => $storage, 'lock_manager' => $this->lockManager, - 'user_session' => $this->userSession, 'lock_service' => $this->lockService, ] ), 0); diff --git a/lib/Model/FileLock.php b/lib/Model/FileLock.php index c95d06f1..a0ede657 100644 --- a/lib/Model/FileLock.php +++ b/lib/Model/FileLock.php @@ -51,14 +51,13 @@ public function __construct() { } /** - * @param int $timeout lifetime in seconds counted from creation, <= 0 for a lock that never expires + * The lock never expires until a caller sets an expiry on it. */ - public static function fromLockScope(LockContext $lockScope, int $timeout): FileLock { + public static function fromLockScope(LockContext $lockScope): FileLock { $lock = new FileLock(); $lock->setUserId($lockScope->getOwner()); $lock->setLockType($lockScope->getType()); $lock->setFileId($lockScope->getNode()->getId()); - $lock->setTimeout($timeout); return $lock; } @@ -241,7 +240,7 @@ public function importFromDatabase(array $data): self { } /** - * Import the shape produced by jsonSerialize() (also accepts database column names). + * Import a lock from the properties a remote DAV storage reports. */ public function import(array $data): void { $this->setId((int)($data['id'] ?? 0)); @@ -251,13 +250,7 @@ public function import(array $data): void { $this->setToken((string)($data['token'] ?? '')); $this->setCreation((int)($data['creation'] ?? 0)); $this->setLockType((int)($data['type'] ?? ILock::TYPE_USER)); - if (array_key_exists('expiresAt', $data)) { - $this->setExpiresAt($data['expiresAt'] === null ? null : (int)$data['expiresAt']); - } elseif (array_key_exists('expires_at', $data)) { - $this->setExpiresAt($data['expires_at'] === null ? null : (int)$data['expires_at']); - } elseif (isset($data['ttl'])) { - $this->setTimeout((int)$data['ttl']); - } + $this->setTimeout((int)($data['ttl'] ?? 0)); $this->setDisplayName((string)($data['displayName'] ?? $data['owner'] ?? '')); } diff --git a/lib/Service/LockService.php b/lib/Service/LockService.php index 9ef5451f..7d3ef5b0 100644 --- a/lib/Service/LockService.php +++ b/lib/Service/LockService.php @@ -24,7 +24,6 @@ use OCP\AppFramework\Services\IAppConfig; use OCP\AppFramework\Utility\ITimeFactory; use OCP\Constants; -use OCP\EventDispatcher\IEventDispatcher; use OCP\Files\File; use OCP\Files\IHomeStorage; use OCP\Files\InvalidPathException; @@ -57,7 +56,6 @@ public function __construct( private readonly FileService $fileService, private readonly IAppConfig $appConfig, private readonly IAppManager $appManager, - IEventDispatcher $eventDispatcher, private readonly IUserSession $userSession, private readonly IRequest $request, private readonly LoggerInterface $logger, @@ -74,10 +72,6 @@ private function now(): int { return Server::get(ITimeFactory::class)->getTime(); } - public function getPolicy(): LockPolicy { - return $this->policy; - } - /** * Register a lock token presented by the current request (WebDAV If header). */ @@ -87,13 +81,6 @@ public function presentToken(string $token): void { } } - /** - * @return list - */ - public function getPresentedTokens(): array { - return $this->presentedTokens; - } - public function resetPresentedTokens(): void { $this->presentedTokens = []; } @@ -110,8 +97,9 @@ public function clearCache(): void { public function getActiveLock(int $fileId): ?FileLock { if (array_key_exists($fileId, $this->lockCache)) { $cached = $this->lockCache[$fileId]; - if ($cached instanceof FileLock && $cached->isExpired($this->now())) { - $this->locksRequest->removeExpired($fileId, $this->now()); + $now = $this->now(); + if ($cached instanceof FileLock && $cached->isExpired($now)) { + $this->locksRequest->removeExpired($fileId, $now); $this->lockCache[$fileId] = false; return null; } @@ -125,19 +113,20 @@ public function getActiveLock(int $fileId): ?FileLock { } } - public function getLockForNodeId(int $nodeId, ?Node $node = null): FileLock|false { + /** + * Lock of a file, falling back to the lock a remote DAV storage reports. + */ + public function getLockForNodeId(int $nodeId, ?Node $node = null): ?FileLock { $lock = $this->getActiveLock($nodeId); if ($lock !== null) { return $lock; } - if (array_key_exists($nodeId, $this->remoteLockCache)) { - return $this->remoteLockCache[$nodeId]; + if (!array_key_exists($nodeId, $this->remoteLockCache)) { + $this->remoteLockCache[$nodeId] = $this->getRemoteLockFromDav($nodeId, $node) ?: false; } - $remoteLock = $this->getRemoteLockFromDav($nodeId, $node); - $this->remoteLockCache[$nodeId] = $remoteLock ?: false; - return $this->remoteLockCache[$nodeId]; + return $this->remoteLockCache[$nodeId] ?: null; } /** @@ -149,7 +138,6 @@ public function getLockForNodeIds(array $nodeIds): array { $locks = []; $locksToRequest = []; foreach ($nodeIds as $nodeId) { - $nodeId = (int)$nodeId; if (array_key_exists($nodeId, $this->lockCache) && $this->lockCache[$nodeId] instanceof FileLock) { $locks[$nodeId] = $this->lockCache[$nodeId]; } elseif (array_key_exists($nodeId, $this->remoteLockCache)) { @@ -162,9 +150,10 @@ public function getLockForNodeIds(array $nodeIds): array { return $locks; } - // pre-fill the cache with negative hits for all requested ids - // so if no lock is found for the file we store the negative hit + // pre-fill with negative hits for all requested ids, so that a file with no + // lock is reported as such instead of being left out of the result foreach ($locksToRequest as $fileId) { + $locks[$fileId] = false; $this->lockCache[$fileId] = false; } @@ -197,7 +186,7 @@ public function getLockForNodeIds(array $nodeIds): array { /** * Configured lock lifetime in seconds, ETA_INFINITE when locks never expire. */ - public function getConfiguredTimeout(): int { + private function getConfiguredTimeout(): int { $minutes = $this->appConfig->getAppValueInt(ConfigLexicon::LOCK_TIMEOUT); return $minutes > 0 ? $minutes * 60 : FileLock::ETA_INFINITE; } @@ -222,24 +211,28 @@ public function lock(LockContext $lockScope): FileLock { * @throws NotFileException */ public function acquire(LockContext $lockScope, ?int $timeout = null, ?string $token = null, ?string $displayName = null, bool $filesOnly = true): FileLock { - $this->canLock($lockScope, null, $filesOnly); + $this->canLock($lockScope, $filesOnly); $fileId = $lockScope->getNode()->getId(); $timeout ??= $this->getConfiguredTimeout(); $now = $this->now(); - $this->locksRequest->removeExpired($fileId, $now); unset($this->lockCache[$fileId]); try { $known = $this->locksRequest->getFromFileId($fileId); - return $this->refreshOrConflict($known, $lockScope, $timeout, $now); + if (!$known->isExpired($now)) { + return $this->refreshOrConflict($known, $lockScope, $timeout, $now); + } + // the delete keeps its own expiry condition, so a refresh that lands in + // between is not dropped + $this->locksRequest->removeExpired($fileId, $now); } catch (LockNotFoundException) { } - $lock = FileLock::fromLockScope($lockScope, 0); + $lock = FileLock::fromLockScope($lockScope); $lock->setCreation($now); $lock->setExpiresAt($timeout > 0 ? $now + $timeout : null); - $lock->setToken($token ?? self::PREFIX . '/' . uuid_create(UUID_TYPE_RANDOM)); + $lock->setToken($token ?? $this->newToken()); if ($displayName !== null) { $lock->setDisplayName($displayName); } else { @@ -249,15 +242,7 @@ public function acquire(LockContext $lockScope, ?int $timeout = null, ?string $t try { $this->locksRequest->save($lock); } catch (LockConflictException) { - $known = null; - try { - $known = $this->locksRequest->getFromFileId($fileId); - } catch (LockNotFoundException) { - // no row for this file, so the unique index that fired was the one on - // the token; take a fresh token and try once more - $lock->setToken(self::PREFIX . '/' . uuid_create(UUID_TYPE_RANDOM)); - $this->locksRequest->save($lock); - } + $known = $this->storeAfterConflict($lock); if ($known !== null) { return $this->refreshOrConflict($known, $lockScope, $timeout, $now); } @@ -269,6 +254,32 @@ public function acquire(LockContext $lockScope, ?int $timeout = null, ?string $t return $lock; } + /** + * Recover from an insert the database refused. + * + * A row for this file means another request won the race. Otherwise the index + * that fired was the one on the token, and a fresh token gets one more + * attempt; losing that one as well means a competing insert landed in + * between, which is a conflict on the file like any other. + * + * @return FileLock|null the lock that won the file, or null once $lock is stored + */ + private function storeAfterConflict(FileLock $lock): ?FileLock { + try { + return $this->locksRequest->getFromFileId($lock->getFileId()); + } catch (LockNotFoundException) { + } + + $lock->setToken($this->newToken()); + try { + $this->locksRequest->save($lock); + } catch (LockConflictException) { + return $this->locksRequest->getFromFileId($lock->getFileId()); + } + + return null; + } + /** * @throws OwnerLockedException */ @@ -286,9 +297,8 @@ private function refreshOrConflict(FileLock $known, LockContext $lockScope, int return $known; } - public function update(FileLock $lock): void { - $this->locksRequest->update($lock); - $this->lockCache[$lock->getFileId()] = $lock; + private function newToken(): string { + return self::PREFIX . '/' . uuid_create(UUID_TYPE_RANDOM); } public function getAppName(string $appId): ?string { @@ -327,21 +337,15 @@ public function unlock(LockContext $lock, bool $force = false, ?string $token = return $known; } - /** - * @deprecated the owner override is part of the policy; kept for callers of older versions - */ - public function enableUserOverride(): void { - } - /** * @throws UnauthorizedUnlockException when the node cannot be locked by the caller * @throws NotFileException when the node is not a file */ - public function canLock(LockContext $request, ?FileLock $current = null, bool $filesOnly = true): void { + public function canLock(LockContext $request, bool $filesOnly = true): void { if ($filesOnly && !$request->getNode() instanceof File) { throw new NotFileException($this->l10n->t('Only files can be locked.')); } - if (($request->getNode()->getPermissions() & Constants::PERMISSION_UPDATE) === 0) { + if (!$this->canModify($request->getNode())) { throw new UnauthorizedUnlockException( $this->l10n->t('File can only be locked with update permissions.') ); @@ -357,23 +361,6 @@ public function canWrite(FileLock $lock, ?LockContext $scope): bool { return $this->policy->canWrite($lock, $this->userSession->getUser()?->getUID(), $this->presentedTokens, $scope); } - /** - * Whether the current user may unlock $current through a request carrying $request. - */ - public function canUnlock(LockContext $request, FileLock $current, ?string $token = null): void { - if (!$this->policy->canUnlock($current, $request, $token, $this->isFileOwner($request->getNode()), false, $this->canModify($request->getNode()))) { - throw new UnauthorizedUnlockException( - $this->l10n->t('File can only be unlocked by the owner of the lock') - ); - } - } - - /** - * The file owner override applies only to files stored in a user's own home - * storage (directly or through a share of it). Group folders, external - * storages and other mounts report the current user as owner of every file, - * so they never grant the override. - */ /** * Whether the current caller may write the node at all, independently of any lock. */ @@ -385,6 +372,12 @@ public function canModify(Node $node): bool { } } + /** + * The file owner override applies only to files stored in a user's own home + * storage (directly or through a share of it). Group folders, external + * storages and other mounts report the current user as owner of every file, + * so they never grant the override. + */ public function isFileOwner(Node $node): bool { $user = $this->userSession->getUser(); if ($user === null) { @@ -476,14 +469,6 @@ public function getExpiredLocks(int $limit = 0): array { } } - /** - * @deprecated use getExpiredLocks() - * @return FileLock[] - */ - public function getDeprecatedLocks(int $limit = 0): array { - return $this->getExpiredLocks($limit); - } - /** * Active lock of a file, removing it first when it has expired. * @@ -507,26 +492,7 @@ public function getLockFromFileId(int $fileId): FileLock { } /** - * Locks on files below a folder that the current user may not write. - * - * @param LockContext|null $scope active ILockManager scope of the caller - * @return list - */ - public function getBlockingLocksBelow(int $folderId, ?LockContext $scope): array { - $now = $this->now(); - $blocking = []; - foreach ($this->locksRequest->getLocksBelow($folderId) as $entry) { - if ($entry['lock']->isExpired($now)) { - continue; - } - if (!$this->canWrite($entry['lock'], $scope)) { - $blocking[] = $entry; - } - } - return $blocking; - } - - /** + * /** * Active locks on files below a folder (relative path included). * * @return list @@ -545,7 +511,7 @@ public function injectMetadata(FileLock $lock): FileLock { $displayName = $this->userManager->getDisplayName($lock->getOwner()); } if ($lock->getType() === ILock::TYPE_APP) { - $displayName = $this->getAppName($lock->getOwner()) ?? null; + $displayName = $this->getAppName($lock->getOwner()); } if ($lock->getType() === ILock::TYPE_TOKEN) { $displayName = $lock->getDisplayName(); @@ -578,14 +544,6 @@ private function getClientHint(): ?string { return null; } - public function generateToken(FileLock $lock): void { - if ($lock->getToken() !== '') { - return; - } - - $lock->setToken(self::PREFIX . '/' . uuid_create(UUID_TYPE_RANDOM)); - } - /** * Remove the given locks, skipping any that are no longer expired because * their owner refreshed them after the batch was read. @@ -607,6 +565,10 @@ public function removeLocksIfExpired(array $locks): void { } /** + * Remove the given locks unconditionally. The cleanup job wants + * removeLocksIfExpired() instead, which will not drop a lock that was + * refreshed after the batch was read. + * * @param FileLock[] $locks */ public function removeLocks(array $locks): void { @@ -637,7 +599,7 @@ public function getRemoteLockFromDav(int $nodeId, ?Node $node = null): ?FileLock $userFolder = $this->rootFolder->getUserFolder($user->getUID()); $node = $userFolder->getFirstNodeById($nodeId); } - if (empty($node)) { + if ($node === null) { return null; } @@ -683,24 +645,6 @@ public function getRemoteLockFromDav(int $nodeId, ?Node $node = null): ?FileLock } } - /** - * Warm the remote property cache of a DAV backed folder with one remote - * listing so that per-file lookups do not trigger remote requests. - */ - public function prefetchRemoteLocks(Node $folder): void { - try { - $storage = $folder->getStorage(); - while ($storage->instanceOfStorage(Wrapper::class)) { - $storage = $storage->getWrapperStorage(); - } - if (!$storage->instanceOfStorage(DAV::class)) { - return; - } - } catch (\Exception $e) { - $this->logger->debug('Failed to prefetch remote locks: ' . $e->getMessage(), ['exception' => $e]); - } - } - private function propagateEtag(Node $node): void { try { $node->getStorage()->getCache()->update($node->getId(), [ diff --git a/lib/Storage/LockWrapper.php b/lib/Storage/LockWrapper.php index b7db3c2a..03594141 100644 --- a/lib/Storage/LockWrapper.php +++ b/lib/Storage/LockWrapper.php @@ -23,15 +23,8 @@ */ class LockWrapper extends Wrapper { private readonly ILockManager $lockManager; + private readonly LockService $lockService; - /** @var LockService */ - private $lockService; - - /** - * LockWrapper constructor. - * - * @param $arguments - */ public function __construct(array $arguments) { parent::__construct($arguments); @@ -40,12 +33,9 @@ public function __construct(array $arguments) { } /** - * @param $path - * @param $permissions - * * @throws LockedException */ - protected function checkPermissions($path, $permissions): bool { + protected function checkPermissions(string $path, int $permissions): bool { if ($permissions === Constants::PERMISSION_READ) { return true; } @@ -69,26 +59,49 @@ protected function checkPermissions($path, $permissions): bool { * Refuse an operation on a directory that would delete or relocate a locked * descendant the current user may not write. * + * @return list the active locks below $path * @throws LockedException */ - protected function checkDescendants(IStorage $storage, string $path): void { + protected function checkDescendants(IStorage $storage, string $path): array { if (!$storage->is_dir($path)) { - return; + return []; } $folderId = $storage->getCache()->getId($path); if ($folderId === -1) { + return []; + } + + $below = $this->lockService->getLocksBelow($folderId); + $scope = $this->lockManager->getLockInScope(); + foreach ($below as $entry) { + $lock = $entry['lock']; + if (!$this->lockService->canWrite($lock, $scope)) { + throw new ManuallyLockedException( + rtrim($path, '/') . '/' . $entry['path'], null, $lock->getToken(), $lock->getOwner(), $lock->getETA() + ); + } + } + + return $below; + } + + /** + * Refuse an operation that would take a locked file out of its source storage. + * + * @throws LockedException + */ + protected function checkSourceLock(IStorage $sourceStorage, string $path): void { + $fileId = $sourceStorage->getCache()->getId($path); + if ($fileId <= 0) { return; } - $blocking = $this->lockService->getBlockingLocksBelow($folderId, $this->lockManager->getLockInScope()); - if ($blocking === []) { + $lock = $this->lockService->getActiveLock($fileId); + if ($lock === null || $this->lockService->canWrite($lock, $this->lockManager->getLockInScope())) { return; } - /** @var FileLock $lock */ - $lock = $blocking[0]['lock']; - throw new ManuallyLockedException( - rtrim($path, '/') . '/' . $blocking[0]['path'], null, $lock->getToken(), $lock->getOwner(), $lock->getETA() - ); + + throw new ManuallyLockedException($path, null, $lock->getToken(), $lock->getOwner(), $lock->getETA()); } #[\Override] @@ -124,13 +137,7 @@ public function copy(string $source, string $target): bool { #[\Override] public function copyFromStorage(IStorage $sourceStorage, string $sourceInternalPath, string $targetInternalPath): bool { - $fileId = $sourceStorage->getCache()->getId($sourceInternalPath); - if ($fileId > 0) { - $lock = $this->lockService->getActiveLock($fileId); - if ($lock !== null && !$this->lockService->canWrite($lock, $this->lockManager->getLockInScope())) { - throw new ManuallyLockedException($sourceInternalPath, null, $lock->getToken(), $lock->getOwner(), $lock->getETA()); - } - } + $this->checkSourceLock($sourceStorage, $sourceInternalPath); return parent::copyFromStorage($sourceStorage, $sourceInternalPath, $targetInternalPath); } @@ -138,13 +145,7 @@ public function copyFromStorage(IStorage $sourceStorage, string $sourceInternalP #[\Override] public function moveFromStorage(IStorage $sourceStorage, string $sourceInternalPath, string $targetInternalPath): bool { $this->checkDescendants($sourceStorage, $sourceInternalPath); - $fileId = $sourceStorage->getCache()->getId($sourceInternalPath); - if ($fileId > 0) { - $lock = $this->lockService->getActiveLock($fileId); - if ($lock !== null && !$this->lockService->canWrite($lock, $this->lockManager->getLockInScope())) { - throw new ManuallyLockedException($sourceInternalPath, null, $lock->getToken(), $lock->getOwner(), $lock->getETA()); - } - } + $this->checkSourceLock($sourceStorage, $sourceInternalPath); $permissions = $this->file_exists($targetInternalPath) ? Constants::PERMISSION_UPDATE : Constants::PERMISSION_CREATE; return $this->checkPermissions($targetInternalPath, $permissions) @@ -166,14 +167,12 @@ public function mkdir(string $path): bool { #[\Override] public function rmdir(string $path): bool { - $this->checkDescendants($this, $path); + $lockedIds = array_map( + fn (array $entry): int => $entry['lock']->getFileId(), + $this->checkDescendants($this, $path) + ); $this->checkPermissions($path, Constants::PERMISSION_DELETE); - $folderId = $this->getCache()->getId($path); - $lockedIds = $folderId > 0 - ? array_map(fn (array $entry): int => $entry['lock']->getFileId(), $this->lockService->getLocksBelow($folderId)) - : []; - $result = parent::rmdir($path); if ($result && $lockedIds !== []) { $this->lockService->removeLocksForFileIds($lockedIds); diff --git a/tests/Feature/AcquisitionTest.php b/tests/Feature/AcquisitionTest.php index ed99bf10..8864cde9 100644 --- a/tests/Feature/AcquisitionTest.php +++ b/tests/Feature/AcquisitionTest.php @@ -35,12 +35,12 @@ public function testDatabaseRejectsSecondLockRow(): void { $file = $this->loginAndGetUserFolder(self::USER1)->newFile('dup.txt', 'AAA'); $request = \OCP\Server::get(LocksRequest::class); - $first = FileLock::fromLockScope(new LockContext($file, ILock::TYPE_USER, self::USER1), 0); + $first = FileLock::fromLockScope(new LockContext($file, ILock::TYPE_USER, self::USER1)); $first->setToken('files_lock/dup-1'); $request->save($first); self::assertGreaterThan(0, $first->getId()); - $second = FileLock::fromLockScope(new LockContext($file, ILock::TYPE_USER, self::USER2), 0); + $second = FileLock::fromLockScope(new LockContext($file, ILock::TYPE_USER, self::USER2)); $second->setToken('files_lock/dup-2'); try { $request->save($second); @@ -50,6 +50,24 @@ public function testDatabaseRejectsSecondLockRow(): void { self::assertSame(1, $this->lockRowCount($file->getId())); } + /** + * The bulk read reports every id it was asked about, so a caller can index the + * result by file id. A file with no lock is reported as false, not left out. + */ + public function testBulkReadReportsEveryRequestedId(): void { + $folder = $this->loginAndGetUserFolder(self::USER1); + $locked = $folder->newFile('bulk-locked.txt', 'AAA'); + $free = $folder->newFile('bulk-free.txt', 'AAA'); + $this->lockManager->lock(new LockContext($locked, ILock::TYPE_USER, self::USER1)); + $this->lockService()->clearCache(); + + $locks = $this->lockService()->getLockForNodeIds([$locked->getId(), $free->getId()]); + + self::assertArrayHasKey($free->getId(), $locks, 'a file with no lock must still be reported'); + self::assertFalse($locks[$free->getId()]); + self::assertInstanceOf(FileLock::class, $locks[$locked->getId()]); + } + public function testConflictReportsWinningLock(): void { $file = $this->sharedFile('conflict.txt'); $mine = $this->lockManager->lock(new LockContext($file, ILock::TYPE_USER, self::USER1)); diff --git a/tests/Feature/LockFeatureTest.php b/tests/Feature/LockFeatureTest.php index 706838b3..d4a63be1 100644 --- a/tests/Feature/LockFeatureTest.php +++ b/tests/Feature/LockFeatureTest.php @@ -243,7 +243,7 @@ public function testExpiredLocksAreDeprecated(): void { // Travel past the 30m timeout window. $this->toTheFuture(30 * 60 + 1); $mapToIds = fn (ILock $deprecatedLock): int => $deprecatedLock->getId(); - $deprecated = array_map($mapToIds, $service->getDeprecatedLocks()); + $deprecated = array_map($mapToIds, $service->getExpiredLocks()); self::assertContains( $lock->getId(), @@ -279,7 +279,7 @@ public function testLockCreationUsesTheInjectedClock(): void { } // Use expired locks to model the cron cleanup workflow: - // getDeprecatedLocks() selects stale locks and removeLocks() deletes them. + // getExpiredLocks() selects stale locks and removeLocks() deletes them. public function testRemoveDeprecatedLocks(): void { $service = \OCP\Server::get(LockService::class); \OCP\Server::get(IConfig::class)->setAppValue(Application::APP_ID, ConfigLexicon::LOCK_TIMEOUT, 30); @@ -289,13 +289,13 @@ public function testRemoveDeprecatedLocks(): void { $lock2 = $this->lockManager->lock(new LockContext($file2, ILock::TYPE_USER, self::TEST_USER1)); $this->toTheFuture(30 * 60 + 1); $mapToIds = fn (ILock $lock): int => $lock->getId(); - $deprecated = array_map($mapToIds, $service->getDeprecatedLocks()); + $deprecated = array_map($mapToIds, $service->getExpiredLocks()); self::assertContains($lock1->getId(), $deprecated); self::assertContains($lock2->getId(), $deprecated); $service->removeLocks([$lock1, $lock2]); - $deprecated = array_map($mapToIds, $service->getDeprecatedLocks()); + $deprecated = array_map($mapToIds, $service->getExpiredLocks()); self::assertNotContains($lock1->getId(), $deprecated); self::assertNotContains($lock2->getId(), $deprecated); From 02b993e7cd6896a74467d62f8d7a7ee6b3c878bf Mon Sep 17 00:00:00 2001 From: Git'Fellow <12234510+solracsf@users.noreply.github.com> Date: Wed, 9 Sep 2026 13:08:06 +0200 Subject: [PATCH 8/8] test: pin the token collision, the stored scope and the client's unlock Two fixes from the audit round shipped without a regression test. The token carries a unique index of its own, so an insert can be refused over a token that another file's lock already holds. That is not a conflict on this file: acquisition takes a fresh token and completes. Before the fix it re-read the file's own row, found nothing, and came back with LockNotFoundException. The scope is written on insert, and rows that predate that hold 0, which is not a valid scope and has to read back as exclusive. The first assertion reads the column directly on purpose, because reading it back through the model would apply the very coercion the second assertion covers. The desktop client takes a lock as a user lock and releases it with whatever type its own journal recorded for the file, so the two disagree as soon as that record is stale. The released app refuses that release with 423, because canUnlock() wants the asserted type to match the stored one and only grants the file owner override when the request itself claims a user lock. The lock then survives, and with lock_timeout at its default of -1 it survives for good, which is what #1230 reports. Asserted through the share recipient rather than the file owner, who is let through before any of that, so it cannot pass for the wrong reason. TEST_FILES had drifted apart from the tests it cleans up after as well: seven files they create were missing from it and two entries named files that no longer exist. One run stays green because each of those seven is created by exactly one test, but the leftovers survive into the next run in the same container, which is what the list is for. Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com> --- tests/Feature/AcquisitionTest.php | 40 +++++++++++++++++++++++++++++++ tests/Feature/DavLockTest.php | 8 +++++++ tests/Feature/LockFeatureTest.php | 9 +++++-- 3 files changed, 55 insertions(+), 2 deletions(-) diff --git a/tests/Feature/AcquisitionTest.php b/tests/Feature/AcquisitionTest.php index 8864cde9..8e76e0b3 100644 --- a/tests/Feature/AcquisitionTest.php +++ b/tests/Feature/AcquisitionTest.php @@ -17,6 +17,7 @@ use OCP\Files\Lock\ILock; use OCP\Files\Lock\LockContext; use OCP\Files\Lock\OwnerLockedException; +use OCP\IDBConnection; use OCP\IUserManager; use OCP\Share\IManager as IShareManager; use OCP\Share\IShare; @@ -50,6 +51,45 @@ public function testDatabaseRejectsSecondLockRow(): void { self::assertSame(1, $this->lockRowCount($file->getId())); } + /** + * The token has a unique index of its own, so an insert can be refused over a + * token another file already holds. That is not a conflict on this file: the + * acquisition takes a fresh token and completes. + */ + public function testTokenCollisionTakesAFreshToken(): void { + $folder = $this->loginAndGetUserFolder(self::USER1); + $taken = $folder->newFile('token-taken.txt', 'AAA'); + $free = $folder->newFile('token-free.txt', 'AAA'); + + $first = $this->lockService()->acquire(new LockContext($taken, ILock::TYPE_TOKEN, self::USER1), null, 'files_lock/collide'); + self::assertSame('files_lock/collide', $first->getToken(), 'without this the second acquisition never collides'); + + $second = $this->lockService()->acquire(new LockContext($free, ILock::TYPE_TOKEN, self::USER1), null, 'files_lock/collide'); + + self::assertNotSame('files_lock/collide', $second->getToken(), 'the colliding token must be replaced'); + self::assertStringStartsWith('files_lock/', $second->getToken()); + self::assertSame(1, $this->lockRowCount($free->getId())); + self::assertSame('files_lock/collide', $this->storedLock($taken->getId())?->getToken(), 'the first lock keeps its token'); + } + + /** + * The scope belongs to the row from the start. Rows written before it was + * stored on insert hold 0, which is not a valid scope, and read back as + * exclusive like every other lock. + */ + public function testScopeIsWrittenOnInsertAndLegacyRowsReadAsExclusive(): void { + $file = $this->loginAndGetUserFolder(self::USER1)->newFile('scope.txt', 'AAA'); + $this->lockManager->lock(new LockContext($file, ILock::TYPE_USER, self::USER1)); + + $connection = \OCP\Server::get(IDBConnection::class); + $stored = (int)$connection->executeQuery('SELECT `scope` FROM `*PREFIX*files_lock` WHERE `file_id` = ?', [$file->getId()])->fetchOne(); + self::assertSame(ILock::LOCK_EXCLUSIVE, $stored, 'read raw, because reading it back through the model would coerce it'); + + $connection->executeStatement('UPDATE `*PREFIX*files_lock` SET `scope` = 0 WHERE `file_id` = ?', [$file->getId()]); + + self::assertSame(ILock::LOCK_EXCLUSIVE, $this->storedLock($file->getId())?->getScope(), 'a row predating the stored scope reads back as exclusive'); + } + /** * The bulk read reports every id it was asked about, so a caller can index the * result by file id. A file with no lock is reported as false, not left out. diff --git a/tests/Feature/DavLockTest.php b/tests/Feature/DavLockTest.php index 645f606e..2dc81df3 100644 --- a/tests/Feature/DavLockTest.php +++ b/tests/Feature/DavLockTest.php @@ -327,6 +327,14 @@ public function testXUserLock(): void { self::assertSame(ILock::TYPE_TOKEN, $this->storedLock($file->getId())?->getType()); self::assertSame(423, $this->request(self::USER2, 'UNLOCK', '/xuser.txt', null, ['X-User-Lock' => '1', 'X-User-Lock-Type' => '2'])->getStatus()); self::assertSame(200, $this->request(self::USER1, 'UNLOCK', '/xuser.txt', null, ['X-User-Lock' => '1', 'X-User-Lock-Type' => '2'])->getStatus()); + + // The desktop client always takes the lock as a user lock but releases it + // with whatever type its own journal recorded, so the two disagree as soon + // as that record goes stale. Asserted through the recipient, who does not + // own the file, because the file owner is let through before any of this. + self::assertSame(200, $this->request(self::USER2, 'LOCK', '/xuser.txt', null, ['X-User-Lock' => '1', 'X-User-Lock-Type' => '0'])->getStatus()); + self::assertSame(200, $this->request(self::USER2, 'UNLOCK', '/xuser.txt', null, ['X-User-Lock' => '1', 'X-User-Lock-Type' => '2'])->getStatus()); + self::assertSame(0, $this->lockRowCount($file->getId()), 'a lock the holder asked to release must not survive'); } public function testAncestorOperationsAreRefusedBeforeMutation(): void { diff --git a/tests/Feature/LockFeatureTest.php b/tests/Feature/LockFeatureTest.php index d4a63be1..798dcea3 100644 --- a/tests/Feature/LockFeatureTest.php +++ b/tests/Feature/LockFeatureTest.php @@ -49,11 +49,16 @@ class LockFeatureTest extends TestCase { 'test-file-creation-clock', 'test-file-dav-infinite', 'test-file-dav-expiring', + 'test-file-extend', + 'test-file-extend-infinite', + 'test-file-remove-lock', + 'test-file-token', 'test-file_public', 'test-file-client', 'etag_test', - 'test-expired-lock-is-deprecated', - 'test-expired-lock-is-deprecated-2', + 'test-expired-lock', + 'test-expired-lock-remove-1', + 'test-expired-lock-remove-2', ]; protected LockManager $lockManager;