Merge pull request #25036 from nextcloud/fix/noid/limitied-allowed-it…

…ems-db-in_2 respect DB restrictions on number of arguments in statements and queries
nextcloud · Jan 14, 2021 · f9ab757 · f9ab757
2 parents 121e2d8 + 21ca5d4
commit f9ab757
Show file tree

Hide file tree

Showing 4 changed files with 178 additions and 12 deletions.
diff --git a/apps/user_ldap/lib/Mapping/AbstractMapping.php b/apps/user_ldap/lib/Mapping/AbstractMapping.php
@@ -30,6 +30,7 @@
 use Doctrine\DBAL\Exception;
 use OC\DB\QueryBuilder\QueryBuilder;
 use OCP\DB\IPreparedStatement;
+use OCP\DB\QueryBuilder\IQueryBuilder;
 
 /**
  * Class AbstractMapping
@@ -199,19 +200,59 @@ public function getNameByDN($fdn) {
 		return $this->cache[$fdn];
 	}
 
-	public function getListOfIdsByDn(array $fdns): array {
+	protected function prepareListOfIdsQuery(array $dnList): IQueryBuilder {
 		$qb = $this->dbc->getQueryBuilder();
 		$qb->select('owncloud_name', 'ldap_dn')
 			->from($this->getTableName(false))
-			->where($qb->expr()->in('ldap_dn', $qb->createNamedParameter($fdns, QueryBuilder::PARAM_STR_ARRAY)));
-		$stmt = $qb->execute();
+			->where($qb->expr()->in('ldap_dn', $qb->createNamedParameter($dnList, QueryBuilder::PARAM_STR_ARRAY)));
+		return $qb;
+	}
 
-		$results = $stmt->fetchAll(\Doctrine\DBAL\FetchMode::ASSOCIATIVE);
-		foreach ($results as $key => $entry) {
-			unset($results[$key]);
+	protected function collectResultsFromListOfIdsQuery(IQueryBuilder $qb, array &$results): void {
+		$stmt = $qb->execute();
+		while ($entry = $stmt->fetch(\Doctrine\DBAL\FetchMode::ASSOCIATIVE)) {
 			$results[$entry['ldap_dn']] = $entry['owncloud_name'];
 			$this->cache[$entry['ldap_dn']] = $entry['owncloud_name'];
 		}
+		$stmt->closeCursor();
+	}
+
+	public function getListOfIdsByDn(array $fdns): array {
+		$totalDBParamLimit = 65000;
+		$sliceSize = 1000;
+		$maxSlices = $totalDBParamLimit / $sliceSize;
+		$results = [];
+
+		$slice = 1;
+		$fdnsSlice = count($fdns) > $sliceSize ? array_slice($fdns, 0, $sliceSize) : $fdns;
+		$qb = $this->prepareListOfIdsQuery($fdnsSlice);
+
+		while (isset($fdnsSlice[999])) {
+			// Oracle does not allow more than 1000 values in the IN list,
+			// but allows slicing
+			$slice++;
+			$fdnsSlice = array_slice($fdns, $sliceSize * ($slice - 1), $sliceSize);
+
+			/** @see https://github.com/vimeo/psalm/issues/4995 */
+			/** @psalm-suppress TypeDoesNotContainType */
+			if (!isset($qb)) {
+				$qb = $this->prepareListOfIdsQuery($fdnsSlice);
+				continue;
+			}
+
+			if (!empty($fdnsSlice)) {
+				$qb->orWhere($qb->expr()->in('ldap_dn', $qb->createNamedParameter($fdnsSlice, QueryBuilder::PARAM_STR_ARRAY)));
+			}
+
+			if ($slice % $maxSlices === 0) {
+				$this->collectResultsFromListOfIdsQuery($qb, $results);
+				unset($qb);
+			}
+		}
+
+		if (isset($qb)) {
+			$this->collectResultsFromListOfIdsQuery($qb, $results);
+		}
 
 		return $results;
 	}

diff --git a/apps/user_ldap/tests/Mapping/AbstractMappingTest.php b/apps/user_ldap/tests/Mapping/AbstractMappingTest.php
@@ -281,4 +281,23 @@ public function testList() {
 		$results = $mapper->getList(1, 1);
 		$this->assertSame(1, count($results));
 	}
+
+	public function testGetListOfIdsByDn() {
+		/** @var AbstractMapping $mapper */
+		list($mapper,) = $this->initTest();
+
+		$listOfDNs = [];
+		for ($i = 0; $i < 66640; $i++) {
+			// Postgres has a limit of 65535 values in a single IN list
+			$name = 'as_' . $i;
+			$dn = 'uid=' . $name . ',dc=example,dc=org';
+			$listOfDNs[] = $dn;
+			if ($i % 20 === 0) {
+				$mapper->map($dn, $name, 'fake-uuid-' . $i);
+			}
+		}
+
+		$result = $mapper->getListOfIdsByDn($listOfDNs);
+		$this->assertSame(66640 / 20, count($result));
+	}
 }
diff --git a/lib/private/DB/QueryBuilder/QueryBuilder.php b/lib/private/DB/QueryBuilder/QueryBuilder.php
@@ -253,6 +253,36 @@ public function execute() {
 			}
 		}
 
+		$numberOfParameters = 0;
+		$hasTooLargeArrayParameter = false;
+		foreach ($this->getParameters() as $parameter) {
+			if (is_array($parameter)) {
+				$count = count($parameter);
+				$numberOfParameters += $count;
+				$hasTooLargeArrayParameter = $hasTooLargeArrayParameter || ($count > 1000);
+			}
+		}
+
+		if ($hasTooLargeArrayParameter) {
+			$exception = new QueryException('More than 1000 expressions in a list are not allowed on Oracle.');
+			$this->logger->logException($exception, [
+				'message' => 'More than 1000 expressions in a list are not allowed on Oracle.',
+				'query' => $this->getSQL(),
+				'level' => ILogger::ERROR,
+				'app' => 'core',
+			]);
+		}
+
+		if ($numberOfParameters > 65535) {
+			$exception = new QueryException('The number of parameters must not exceed 65535. Restriction by PostgreSQL.');
+			$this->logger->logException($exception, [
+				'message' => 'The number of parameters must not exceed 65535. Restriction by PostgreSQL.',
+				'query' => $this->getSQL(),
+				'level' => ILogger::ERROR,
+				'app' => 'core',
+			]);
+		}
+
 		$result = $this->queryBuilder->execute();
 		if (is_int($result)) {
 			return $result;

diff --git a/tests/lib/DB/QueryBuilder/QueryBuilderTest.php b/tests/lib/DB/QueryBuilder/QueryBuilderTest.php
@@ -22,6 +22,8 @@
 namespace Test\DB\QueryBuilder;
 
 use Doctrine\DBAL\Query\Expression\CompositeExpression;
+use Doctrine\DBAL\Query\QueryException;
+use Doctrine\DBAL\Result;
 use OC\DB\QueryBuilder\Literal;
 use OC\DB\QueryBuilder\Parameter;
 use OC\DB\QueryBuilder\QueryBuilder;
@@ -1261,6 +1263,10 @@ public function testExecuteWithoutLogger() {
 			->expects($this->once())
 			->method('execute')
 			->willReturn(3);
+		$queryBuilder
+			->expects($this->any())
+			->method('getParameters')
+			->willReturn([]);
 		$this->logger
 			->expects($this->never())
 			->method('debug');
@@ -1277,14 +1283,14 @@ public function testExecuteWithoutLogger() {
 	public function testExecuteWithLoggerAndNamedArray() {
 		$queryBuilder = $this->createMock(\Doctrine\DBAL\Query\QueryBuilder::class);
 		$queryBuilder
-			->expects($this->at(0))
+			->expects($this->any())
 			->method('getParameters')
 			->willReturn([
 				'foo' => 'bar',
 				'key' => 'value',
 			]);
 		$queryBuilder
-			->expects($this->at(1))
+			->expects($this->any())
 			->method('getSQL')
 			->willReturn('SELECT * FROM FOO WHERE BAR = ?');
 		$queryBuilder
@@ -1315,11 +1321,11 @@ public function testExecuteWithLoggerAndNamedArray() {
 	public function testExecuteWithLoggerAndUnnamedArray() {
 		$queryBuilder = $this->createMock(\Doctrine\DBAL\Query\QueryBuilder::class);
 		$queryBuilder
-			->expects($this->at(0))
+			->expects($this->any())
 			->method('getParameters')
 			->willReturn(['Bar']);
 		$queryBuilder
-			->expects($this->at(1))
+			->expects($this->any())
 			->method('getSQL')
 			->willReturn('SELECT * FROM FOO WHERE BAR = ?');
 		$queryBuilder
@@ -1350,11 +1356,11 @@ public function testExecuteWithLoggerAndUnnamedArray() {
 	public function testExecuteWithLoggerAndNoParams() {
 		$queryBuilder = $this->createMock(\Doctrine\DBAL\Query\QueryBuilder::class);
 		$queryBuilder
-			->expects($this->at(0))
+			->expects($this->any())
 			->method('getParameters')
 			->willReturn([]);
 		$queryBuilder
-			->expects($this->at(1))
+			->expects($this->any())
 			->method('getSQL')
 			->willReturn('SELECT * FROM FOO WHERE BAR = ?');
 		$queryBuilder
@@ -1380,4 +1386,74 @@ public function testExecuteWithLoggerAndNoParams() {
 		$this->invokePrivate($this->queryBuilder, 'queryBuilder', [$queryBuilder]);
 		$this->assertEquals(3, $this->queryBuilder->execute());
 	}
+
+	public function testExecuteWithParameterTooLarge() {
+		$queryBuilder = $this->createMock(\Doctrine\DBAL\Query\QueryBuilder::class);
+		$p = array_fill(0, 1001, 'foo');
+		$queryBuilder
+			->expects($this->any())
+			->method('getParameters')
+			->willReturn([$p]);
+		$queryBuilder
+			->expects($this->any())
+			->method('getSQL')
+			->willReturn('SELECT * FROM FOO WHERE BAR IN (?)');
+		$queryBuilder
+			->expects($this->once())
+			->method('execute')
+			->willReturn($this->createMock(Result::class));
+		$this->logger
+			->expects($this->once())
+			->method('logException')
+			->willReturnCallback(function ($e, $parameters) {
+				$this->assertInstanceOf(QueryException::class, $e);
+				$this->assertSame(
+					'More than 1000 expressions in a list are not allowed on Oracle.',
+					$parameters['message']
+				);
+			});
+		$this->config
+			->expects($this->once())
+			->method('getValue')
+			->with('log_query', false)
+			->willReturn(false);
+
+		$this->invokePrivate($this->queryBuilder, 'queryBuilder', [$queryBuilder]);
+		$this->queryBuilder->execute();
+	}
+
+	public function testExecuteWithParametersTooMany() {
+		$queryBuilder = $this->createMock(\Doctrine\DBAL\Query\QueryBuilder::class);
+		$p = array_fill(0, 999, 'foo');
+		$queryBuilder
+			->expects($this->any())
+			->method('getParameters')
+			->willReturn(array_fill(0, 66, $p));
+		$queryBuilder
+			->expects($this->any())
+			->method('getSQL')
+			->willReturn('SELECT * FROM FOO WHERE BAR IN (?) OR BAR IN (?)');
+		$queryBuilder
+			->expects($this->once())
+			->method('execute')
+			->willReturn($this->createMock(Result::class));
+		$this->logger
+			->expects($this->once())
+			->method('logException')
+			->willReturnCallback(function ($e, $parameters) {
+				$this->assertInstanceOf(QueryException::class, $e);
+				$this->assertSame(
+					'The number of parameters must not exceed 65535. Restriction by PostgreSQL.',
+					$parameters['message']
+				);
+			});
+		$this->config
+			->expects($this->once())
+			->method('getValue')
+			->with('log_query', false)
+			->willReturn(false);
+
+		$this->invokePrivate($this->queryBuilder, 'queryBuilder', [$queryBuilder]);
+		$this->queryBuilder->execute();
+	}
 }