diff --git a/lib/private/Lock/Persistent/LockMapper.php b/lib/private/Lock/Persistent/LockMapper.php index 37e29ef19ace..e84009f99f93 100644 --- a/lib/private/Lock/Persistent/LockMapper.php +++ b/lib/private/Lock/Persistent/LockMapper.php @@ -23,6 +23,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 */ @@ -33,54 +35,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($storageId, $internalPath, $returnChildLocks) { $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; } /** @@ -115,23 +180,25 @@ public function getLockByToken($token) { 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 efd1a3a8e3ed..cf6ddbecb237 100644 --- a/lib/public/Lock/Persistent/ILock.php +++ b/lib/public/Lock/Persistent/ILock.php @@ -29,6 +29,7 @@ interface ILock { // these values are in sync with \Sabre\DAV\Locks\LockInfo const LOCK_SCOPE_EXCLUSIVE = 1; const LOCK_SCOPE_SHARED = 2; + const LOCK_DEPTH_ZERO = 0; 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 dbbd184d9788..a374a85e4ddd 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(); + }); + } }