From 0f87cf0c93ecf414653c3927f6b7c7cc6e8b5efa Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Thu, 13 Dec 2018 17:27:05 +0100 Subject: [PATCH] Fix persistent lock retrieval by path Fix query to make it strictly search in the given storage.. Fix query to prevent LIKE bleeding into unrelated folders (trailing slash issue). Added enforcement of Depth 0 or -1 as per RFC 4918 Section 9.10.3. Added many more unit tests to test the query from different angles, making sure that locks from parent and child paths are returned when the correct conditions are met and also not returned when not met. Reenable matching acceptance test --- lib/private/Lock/Persistent/LockMapper.php | 133 ++++++--- lib/public/Lock/Persistent/ILock.php | 1 + .../features/webUIWebdavLocks/locks.feature | 1 - tests/lib/Lock/Persistent/LockMapperTest.php | 255 +++++++++++++++--- 4 files changed, 325 insertions(+), 65 deletions(-) diff --git a/lib/private/Lock/Persistent/LockMapper.php b/lib/private/Lock/Persistent/LockMapper.php index ef9ac5f5dfb2..ba06d68cbbe9 100644 --- a/lib/private/Lock/Persistent/LockMapper.php +++ b/lib/private/Lock/Persistent/LockMapper.php @@ -24,6 +24,8 @@ use OCP\AppFramework\Db\Mapper; use OCP\AppFramework\Utility\ITimeFactory; use OCP\IDBConnection; +use OCP\DB\QueryBuilder\IQueryBuilder; +use OCP\Lock\Persistent\ILock; class LockMapper extends Mapper { /** @var ITimeFactory */ @@ -34,54 +36,117 @@ public function __construct(IDBConnection $db, ITimeFactory $timeFactory) { $this->timeFactory = $timeFactory; } + /** + * Returns an array of all paths to each of + * the parents from the given path. + * + * Ex: if "/a/b/c" is given, returns ["/a", "/a/b"] + * + * @param string $path + * @return string[] array of parent paths + */ + private function getParentPaths($path) { + // We need to check locks for every part in the uri. + $uriParts = \explode('/', $path); + + // only return parents, not the current one + \array_pop($uriParts); + + $parentPaths = []; + + $currentPath = ''; + foreach ($uriParts as $part) { + if ($currentPath) { + $currentPath .= '/'; + } + $currentPath .= $part; + $parentPaths[] = $currentPath; + } + + return $parentPaths; + } + /** * Selects all locks from the database for the given storage and path. - * Also parent folders are returned and in case $returnChildLocks is true all - * children locks as well. + * Locks for parent folders are returned as well as long as they have a non-zero depth, + * as these would mean the target $internalPath is indirectly locked through these. + * In case $returnChildLocks is true, locks for all children paths will be return as well, + * regardless of depth.. + * + * Examples: + * + * When function called with "foo/bar" the returned array will have lock + * entries for the following, given the conditions are met: + * - "foo" if the latter has a lock set with non-zero Depth + * - "foo/bar" if the latter has a lock set (current target) + * - "foo/bar/sub" if the latter has a lock set, and given $returnChildLocks is true * - * @param int $storageId - * @param string $internalPath - * @param bool $returnChildLocks + * @param int $storageId numeric id of the storage for which to retrieve lock entries + * @param string $internalPath target internal path + * @param bool $returnChildLocks whether to return any lock for any child * @return Lock[] */ public function getLocksByPath(int $storageId, string $internalPath, bool $returnChildLocks) : array { $query = $this->db->getQueryBuilder(); - $pathPattern = $this->db->escapeLikeParameter($internalPath) . '%'; - + $internalPath = \rtrim($internalPath, '/'); + + /* + * SELECT `id`, `owner`, `timeout`, `created_at`, `token`, `token`, `scope`, `depth`, `file_id`, `path`, `owner_account_id` + * FROM `oc_persistent_locks` l + * INNER JOIN `oc_filecache` f ON l.`file_id` = f.`fileid` + * WHERE ( + * (`storage` = 4) + * AND (`created_at` > (1544710587 - `timeout`)) + * AND ( + * (f.`path` = 'files/test/target') + * OR ((`depth` <> 0) AND (`path` in ('files', 'files/test'))) + * ) + * ); + */ $query->select(['id', 'owner', 'timeout', 'created_at', 'token', 'token', 'scope', 'depth', 'file_id', 'path', 'owner_account_id']) ->from($this->getTableName(), 'l') ->join('l', 'filecache', 'f', $query->expr()->eq('l.file_id', 'f.fileid')) + // WHERE (`storage` = 4) ->where($query->expr()->eq('storage', $query->createPositionalParameter($storageId))) + // AND (`created_at` > (1544710587 - `timeout`)) ->andWhere($query->expr()->gt('created_at', $query->createFunction('(' . $query->createPositionalParameter($this->timeFactory->getTime()) . ' - `timeout`)'))); + $pathMatchClauses = $query->expr()->orX( + // direct match + // (f.`path` = 'files/test/target') + $query->expr()->eq('f.path', $query->createPositionalParameter($internalPath)) + ); + if ($returnChildLocks) { - $query->andWhere($query->expr()->like('f.path', $query->createPositionalParameter($pathPattern))); - } else { - $query->andWhere($query->expr()->eq('f.path', $query->createPositionalParameter($internalPath))); + $pathMatchClauses->add( + // match all children paths from the current path + // (f.`path` LIKE 'files/test/target/%') + $query->expr()->like('f.path', $query->createPositionalParameter($this->db->escapeLikeParameter($internalPath) . '/%')) + ); } - // We need to check locks for every part in the uri. - $uriParts = \explode('/', $internalPath); - - // We already covered the last part of the uri - \array_pop($uriParts); - - $currentPath = ''; - foreach ($uriParts as $part) { - if ($currentPath) { - $currentPath .= '/'; - } - $currentPath .= $part; - $query->orWhere( + $parentPaths = $this->getParentPaths($internalPath); + if (!empty($parentPaths)) { + // match any parents with the condition that there is a lock with non-zero Depth + $pathMatchClauses->add( $query->expr()->andX( - // TODO: think about parent locks for depth 1 $query->expr()->neq('depth', $query->createPositionalParameter(0)), - $query->expr()->eq('path', $query->createPositionalParameter($currentPath)) + // here we are assuming that the number of path sections will be less than 1000 + $query->expr()->in('path', $query->createPositionalParameter($parentPaths, IQueryBuilder::PARAM_STR_ARRAY)) ) ); } - return $this->findEntities($query->getSQL(), $query->getParameters()); + $query->andWhere($pathMatchClauses); + + $stmt = $query->execute(); + $entities = []; + while ($row = $stmt->fetch()) { + $entities[] = $this->mapRowToEntity($row); + } + $stmt->closeCursor(); + + return $entities; } /** @@ -116,23 +181,25 @@ public function getLockByToken(string $token) : Lock { return $this->findEntity($query->getSQL(), $query->getParameters()); } - public function insert(Entity $entity) { + private function validateEntity($entity) { if (!$entity instanceof Lock) { throw new \InvalidArgumentException('Wrong entity type used'); } if (\md5($entity->getToken()) !== $entity->getTokenHash()) { throw new \InvalidArgumentException('token_hash does not match the token of the lock'); } + if ($entity->getDepth() !== ILock::LOCK_DEPTH_ZERO && $entity->getDepth() !== ILock::LOCK_DEPTH_INFINITE) { + throw new \InvalidArgumentException('Only -1 (infinity) and 0 are supported for lock depth, ' . $entity->getDepth() . ' given'); + } + } + + public function insert(Entity $entity) { + $this->validateEntity($entity); return parent::insert($entity); } public function update(Entity $entity) { - if (!$entity instanceof Lock) { - throw new \InvalidArgumentException('Wrong entity type used'); - } - if (\md5($entity->getToken()) !== $entity->getTokenHash()) { - throw new \InvalidArgumentException('token_hash does not match the token of the lock'); - } + $this->validateEntity($entity); return parent::update($entity); } diff --git a/lib/public/Lock/Persistent/ILock.php b/lib/public/Lock/Persistent/ILock.php index b4fd219c6bdd..c40e023b9af5 100644 --- a/lib/public/Lock/Persistent/ILock.php +++ b/lib/public/Lock/Persistent/ILock.php @@ -30,6 +30,7 @@ interface ILock { // these values are in sync with \Sabre\DAV\Locks\LockInfo public const LOCK_SCOPE_EXCLUSIVE = 1; public const LOCK_SCOPE_SHARED = 2; + public const LOCK_DEPTH_ZERO = 0; public const LOCK_DEPTH_INFINITE = -1; /** diff --git a/tests/acceptance/features/webUIWebdavLocks/locks.feature b/tests/acceptance/features/webUIWebdavLocks/locks.feature index fadf39283df9..0e12d41e1115 100644 --- a/tests/acceptance/features/webUIWebdavLocks/locks.feature +++ b/tests/acceptance/features/webUIWebdavLocks/locks.feature @@ -671,7 +671,6 @@ Feature: Locks | exclusive | | shared | - @skip @issue-33885 Scenario Outline: creating a subfolder structure that is the same as the structure of a declined & locked share Given these users have been created: |username | diff --git a/tests/lib/Lock/Persistent/LockMapperTest.php b/tests/lib/Lock/Persistent/LockMapperTest.php index f1a25bdef009..513d1c6ff9be 100644 --- a/tests/lib/Lock/Persistent/LockMapperTest.php +++ b/tests/lib/Lock/Persistent/LockMapperTest.php @@ -43,12 +43,26 @@ class LockMapperTest extends TestCase { private $account; /** @var int */ private $fileCacheId; + /** @var int */ + private $fileCacheChildId; + /** @var int */ + private $fileCacheParentId; + /** @var int */ + private $storageId; + /** @var int */ + private $unrelatedStorageId; /** @var LockMapper */ private $mapper; /** @var Lock[] */ private $locks = []; /** @var string */ + private $parentPath; + /** @var string */ private $path; + /** @var string */ + private $childPath; + /** @var string */ + private $unrelatedPath; /** @var ITimeFactory */ private $timeFactory; @@ -57,17 +71,23 @@ public function setUp() { $this->db = \OC::$server->getDatabaseConnection(); - // insert test entity in file cache - $insertFileCache = $this->db->getQueryBuilder(); - $this->path = \uniqid('/foo_foo/bar', true); - $insertFileCache->insert('filecache') - ->values([ - 'storage' => 666, - 'path' => $insertFileCache->createNamedParameter($this->path), - 'path_hash' => $insertFileCache->createNamedParameter(\md5($this->path)) - ]) - ->execute(); - $this->fileCacheId = $insertFileCache->getLastInsertId(); + $this->storageId = 666; + $this->unrelatedStorageId = 667; + $this->parentPath = 'foo_foo'; + $this->path = 'foo_foo/bar'; + $this->childPath = 'foo_foo/bar/child'; + // checking for trailing slash issues + $this->unrelatedPath = 'foo_f'; + + // insert test entities in file cache + $this->fileCacheParentId = $this->insertFileCacheEntry($this->storageId, $this->parentPath); + $this->fileCacheId = $this->insertFileCacheEntry($this->storageId, $this->path); + $this->fileCacheChildId = $this->insertFileCacheEntry($this->storageId, $this->childPath); + + // unrelated entries + $this->insertFileCacheEntry($this->unrelatedStorageId, $this->parentPath); + $this->insertFileCacheEntry($this->unrelatedStorageId, $this->path); + $this->insertFileCacheEntry($this->storageId, $this->unrelatedPath); // insert test entity in account table $this->account = new Account(); @@ -104,7 +124,10 @@ protected function tearDown() { $q = $this->db->getQueryBuilder(); $q->delete('filecache') - ->where($q->expr()->eq('fileid', $q->createNamedParameter($this->fileCacheId))) + ->where($q->expr()->eq('storage', $q->createNamedParameter($this->storageId))) + ->execute(); + $q->delete('filecache') + ->where($q->expr()->eq('storage', $q->createNamedParameter($this->unrelatedStorageId))) ->execute(); \OC::$server->getAccountMapper() @@ -114,31 +137,164 @@ protected function tearDown() { } public function testInsert() { - $lock = new Lock(); - $token = \uniqid('tok', true); - $lock->setFileId($this->fileCacheId); - $lock->setToken($token); - $lock->setCreatedAt(\time()); - $lock->setTimeout(1880); - $lock->setScope(ILock::LOCK_SCOPE_EXCLUSIVE); - $lock->setOwnerAccountId($this->account->getId()); - $lock->setDepth(0); - $this->mapper->insert($lock); + $lock = $this->insertLock($this->fileCacheId, ILock::LOCK_SCOPE_EXCLUSIVE, -1); - $this->locks[]= $lock; + $this->locks[] = $lock; - $l = $this->mapper->getLockByToken($token); + $l = $this->mapper->getLockByToken($lock->getToken()); $this->assertLock($lock, $l); - $l = $this->mapper->getLocksByPath(666, $this->path, false); - $this->assertLock($lock, $l[0]); + $this->mapper->deleteByFileIdAndToken($this->fileCacheId, $lock->getToken()); + $l = $this->mapper->getLocksByPath($this->storageId, $this->path, false); + $this->assertCount(0, $l); + } - $l = $this->mapper->getLocksByPath(666, $this->path, true); - $this->assertLock($lock, $l[0]); + /** + * Test that locks the target path and verifies + * whether querying the lock on the target path and + * parent paths + */ + public function testGetLocksByPathDepth0() { + $lock = $this->insertLock($this->fileCacheId, ILock::LOCK_SCOPE_EXCLUSIVE, ILock::LOCK_DEPTH_ZERO); - $this->mapper->deleteByFileIdAndToken($this->fileCacheId, $token); - $l = $this->mapper->getLocksByPath(666, $this->path, false); - $this->assertCount(0, $l); + $this->locks[]= $lock; + + $l = $this->mapper->getLocksByPath($this->storageId, $this->path, false); + $this->assertCount(1, $l); + $this->assertLock($lock, $l[0], 'query on locked path returns lock from locked path itself'); + + $l = $this->mapper->getLocksByPath($this->storageId, $this->parentPath, false); + $this->assertCount(0, $l, 'query on parent path returns no lock'); + + $l = $this->mapper->getLocksByPath($this->storageId, $this->path, true); + $this->assertCount(1, $l); + $this->assertLock($lock, $l[0], 'query on locked path including children returns lock from locked path itself'); + + // parent is able to retrieve for children when asking for children + $l = $this->mapper->getLocksByPath($this->storageId, $this->parentPath, true); + $this->assertCount(1, $l); + $this->assertLock($lock, $l[0], 'query on parent path and including children returns lock from locked path'); + + // unrelated storage with same paths + $l = $this->mapper->getLocksByPath($this->unrelatedStorageId, $this->path, false); + $this->assertEmpty($l, 'query on unrelated storage yields no result'); + + $l = $this->mapper->getLocksByPath($this->unrelatedStorageId, $this->path, true); + $this->assertEmpty($l, 'query on unrelated storage yields no result'); + + $l = $this->mapper->getLocksByPath($this->unrelatedStorageId, $this->parentPath, false); + $this->assertEmpty($l, 'query on unrelated storage yields no result'); + + $l = $this->mapper->getLocksByPath($this->unrelatedStorageId, $this->parentPath, true); + $this->assertEmpty($l, 'query on unrelated storage yields no result'); + + // query unrelated but similar looking parent + $l = $this->mapper->getLocksByPath($this->storageId, $this->unrelatedPath, true); + $this->assertCount(0, $l, 'query on unrelated parent path including children does not mistakenly match the other child'); + } + + /** + * Test that locks the parent folder with infinite depth and + * checks whether querying locks on parent or child returns said lock. + */ + public function testGetLocksByPathDepthInfinity() { + $lock = $this->insertLock($this->fileCacheParentId, ILock::LOCK_SCOPE_EXCLUSIVE, ILock::LOCK_DEPTH_INFINITE); + + $this->locks[] = $lock; + + $l = $this->mapper->getLocksByPath($this->storageId, $this->path, false); + $this->assertCount(1, $l); + $this->assertLock($lock, $l[0], 'query on child path returns lock from parent due to infinite depth'); + + $l = $this->mapper->getLocksByPath($this->storageId, $this->parentPath, false); + $this->assertCount(1, $l); + $this->assertLock($lock, $l[0], 'query on parent path returns lock from parent path itself'); + + $l = $this->mapper->getLocksByPath($this->storageId, $this->path, true); + $this->assertCount(1, $l); + $this->assertLock($lock, $l[0], 'query on child path including children returns lock from parent path due to infinite depth'); + + $l = $this->mapper->getLocksByPath($this->storageId, $this->parentPath, true); + $this->assertCount(1, $l); + $this->assertLock($lock, $l[0], 'query on parent path including children returns lock from parent path itself'); + + // unrelated storage with same paths + $l = $this->mapper->getLocksByPath($this->unrelatedStorageId, $this->path, false); + $this->assertEmpty($l, 'query on unrelated storage yields no result'); + + $l = $this->mapper->getLocksByPath($this->unrelatedStorageId, $this->path, true); + $this->assertEmpty($l, 'query on unrelated storage yields no result'); + + $l = $this->mapper->getLocksByPath($this->unrelatedStorageId, $this->parentPath, false); + $this->assertEmpty($l, 'query on unrelated storage yields no result'); + + $l = $this->mapper->getLocksByPath($this->unrelatedStorageId, $this->parentPath, true); + $this->assertEmpty($l, 'query on unrelated storage yields no result'); + } + + /** + * Test that we are able to retrieve multiple locks for a given path, + * and that existing child locks are NOT returned. + */ + public function testGetLocksByPathWithoutChildren() { + $parentLock = $this->insertLock($this->fileCacheParentId, ILock::LOCK_SCOPE_SHARED, -1); + $lock = $this->insertLock($this->fileCacheId, ILock::LOCK_SCOPE_SHARED, 0); + $childLock = $this->insertLock($this->fileCacheChildId, ILock::LOCK_SCOPE_SHARED, 0); + + $this->locks[] = $parentLock; + $this->locks[] = $lock; + $this->locks[] = $childLock; + + $l = $this->mapper->getLocksByPath($this->storageId, $this->path, false); + $this->sortLocks($l); + $this->assertCount(2, $l); + $this->assertLock($parentLock, $l[0], 'parent lock returned due to infinite depth'); + $this->assertLock($lock, $l[1], 'path lock returned due to it being direct target'); + + $l = $this->mapper->getLocksByPath($this->storageId, $this->parentPath, false); + $this->assertCount(1, $l); + $this->assertLock($parentLock, $l[0], 'parent lock returned due to it being direct target'); + + $l = $this->mapper->getLocksByPath($this->storageId, $this->childPath, false); + $this->sortLocks($l); + $this->assertCount(2, $l); + $this->assertLock($parentLock, $l[0], 'parent lock returned due to infinite depth'); + // $lock not included because it has Depth 0 + $this->assertLock($childLock, $l[1], 'child lock returned'); + } + + /** + * Test that we are able to retrieve multiple locks for a given path + */ + public function testGetLocksByPathMultipleWithChildren() { + $parentLock = $this->insertLock($this->fileCacheParentId, ILock::LOCK_SCOPE_SHARED, -1); + $lock = $this->insertLock($this->fileCacheId, ILock::LOCK_SCOPE_SHARED, 0); + $childLock = $this->insertLock($this->fileCacheChildId, ILock::LOCK_SCOPE_SHARED, 0); + + $this->locks[] = $parentLock; + $this->locks[] = $lock; + $this->locks[] = $childLock; + + $l = $this->mapper->getLocksByPath($this->storageId, $this->path, true); + $this->sortLocks($l); + $this->assertCount(3, $l); + $this->assertLock($parentLock, $l[0], 'parent lock returned due to infinite depth'); + $this->assertLock($lock, $l[1], 'path lock returned due to it being direct target'); + $this->assertLock($childLock, $l[2], 'child lock returned'); + + $l = $this->mapper->getLocksByPath($this->storageId, $this->parentPath, true); + $this->sortLocks($l); + $this->assertCount(3, $l); + $this->assertLock($parentLock, $l[0], 'parent lock returned due to it being direct target'); + $this->assertLock($lock, $l[1], 'path lock returned due to it being a child'); + $this->assertLock($childLock, $l[2], 'child lock returned'); + + $l = $this->mapper->getLocksByPath($this->storageId, $this->childPath, true); + $this->sortLocks($l); + $this->assertCount(2, $l); + $this->assertLock($parentLock, $l[0], 'parent lock returned due to infinite depth'); + // $lock not included because it has Depth 0 + $this->assertLock($childLock, $l[1], 'child lock returned'); } /** @@ -239,4 +395,41 @@ public function testDeleteUserDeletesLock() { $this->mapper->getLockByToken($token); } + + private function insertFileCacheEntry($storage, $path) { + $insertFileCache = $this->db->getQueryBuilder(); + $insertFileCache->insert('filecache') + ->values([ + 'storage' => $insertFileCache->createNamedParameter($storage), + 'name' => $insertFileCache->createNamedParameter(\basename($path)), + 'path' => $insertFileCache->createNamedParameter($path), + 'path_hash' => $insertFileCache->createNamedParameter(\md5($path)) + ]) + ->execute(); + return $insertFileCache->getLastInsertId(); + } + + private function insertLock($fileId, $scope, $depth = 0) { + $lock = new Lock(); + $token = \uniqid('tok', true); + $lock->setFileId($fileId); + $lock->setToken($token); + $lock->setCreatedAt(\time()); + $lock->setTimeout(1880); + $lock->setScope($scope); + $lock->setOwnerAccountId($this->account->getId()); + $lock->setDepth($depth); + $this->mapper->insert($lock); + + return $lock; + } + + /** + * Sorts an array of locks by file id for easier matching + */ + private function sortLocks(&$l) { + \usort($l, function ($a, $b) { + return $a->getId() - $b->getId(); + }); + } }