Merge pull request #25036 from nextcloud/fix/noid/limitied-allowed-items-db-in_2

respect DB restrictions on number of arguments in statements and queries
This commit is contained in:
blizzz 2021-01-14 11:36:42 +01:00 committed by GitHub
commit f9ab7575e7
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 178 additions and 12 deletions

View File

@ -30,6 +30,7 @@ namespace OCA\User_LDAP\Mapping;
use Doctrine\DBAL\Exception; use Doctrine\DBAL\Exception;
use OC\DB\QueryBuilder\QueryBuilder; use OC\DB\QueryBuilder\QueryBuilder;
use OCP\DB\IPreparedStatement; use OCP\DB\IPreparedStatement;
use OCP\DB\QueryBuilder\IQueryBuilder;
/** /**
* Class AbstractMapping * Class AbstractMapping
@ -199,19 +200,59 @@ abstract class AbstractMapping {
return $this->cache[$fdn]; return $this->cache[$fdn];
} }
public function getListOfIdsByDn(array $fdns): array { protected function prepareListOfIdsQuery(array $dnList): IQueryBuilder {
$qb = $this->dbc->getQueryBuilder(); $qb = $this->dbc->getQueryBuilder();
$qb->select('owncloud_name', 'ldap_dn') $qb->select('owncloud_name', 'ldap_dn')
->from($this->getTableName(false)) ->from($this->getTableName(false))
->where($qb->expr()->in('ldap_dn', $qb->createNamedParameter($fdns, QueryBuilder::PARAM_STR_ARRAY))); ->where($qb->expr()->in('ldap_dn', $qb->createNamedParameter($dnList, QueryBuilder::PARAM_STR_ARRAY)));
$stmt = $qb->execute(); return $qb;
}
$results = $stmt->fetchAll(\Doctrine\DBAL\FetchMode::ASSOCIATIVE); protected function collectResultsFromListOfIdsQuery(IQueryBuilder $qb, array &$results): void {
foreach ($results as $key => $entry) { $stmt = $qb->execute();
unset($results[$key]); while ($entry = $stmt->fetch(\Doctrine\DBAL\FetchMode::ASSOCIATIVE)) {
$results[$entry['ldap_dn']] = $entry['owncloud_name']; $results[$entry['ldap_dn']] = $entry['owncloud_name'];
$this->cache[$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; return $results;
} }

View File

@ -281,4 +281,23 @@ abstract class AbstractMappingTest extends \Test\TestCase {
$results = $mapper->getList(1, 1); $results = $mapper->getList(1, 1);
$this->assertSame(1, count($results)); $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));
}
} }

View File

@ -253,6 +253,36 @@ class QueryBuilder implements IQueryBuilder {
} }
} }
$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(); $result = $this->queryBuilder->execute();
if (is_int($result)) { if (is_int($result)) {
return $result; return $result;

View File

@ -22,6 +22,8 @@
namespace Test\DB\QueryBuilder; namespace Test\DB\QueryBuilder;
use Doctrine\DBAL\Query\Expression\CompositeExpression; use Doctrine\DBAL\Query\Expression\CompositeExpression;
use Doctrine\DBAL\Query\QueryException;
use Doctrine\DBAL\Result;
use OC\DB\QueryBuilder\Literal; use OC\DB\QueryBuilder\Literal;
use OC\DB\QueryBuilder\Parameter; use OC\DB\QueryBuilder\Parameter;
use OC\DB\QueryBuilder\QueryBuilder; use OC\DB\QueryBuilder\QueryBuilder;
@ -1261,6 +1263,10 @@ class QueryBuilderTest extends \Test\TestCase {
->expects($this->once()) ->expects($this->once())
->method('execute') ->method('execute')
->willReturn(3); ->willReturn(3);
$queryBuilder
->expects($this->any())
->method('getParameters')
->willReturn([]);
$this->logger $this->logger
->expects($this->never()) ->expects($this->never())
->method('debug'); ->method('debug');
@ -1277,14 +1283,14 @@ class QueryBuilderTest extends \Test\TestCase {
public function testExecuteWithLoggerAndNamedArray() { public function testExecuteWithLoggerAndNamedArray() {
$queryBuilder = $this->createMock(\Doctrine\DBAL\Query\QueryBuilder::class); $queryBuilder = $this->createMock(\Doctrine\DBAL\Query\QueryBuilder::class);
$queryBuilder $queryBuilder
->expects($this->at(0)) ->expects($this->any())
->method('getParameters') ->method('getParameters')
->willReturn([ ->willReturn([
'foo' => 'bar', 'foo' => 'bar',
'key' => 'value', 'key' => 'value',
]); ]);
$queryBuilder $queryBuilder
->expects($this->at(1)) ->expects($this->any())
->method('getSQL') ->method('getSQL')
->willReturn('SELECT * FROM FOO WHERE BAR = ?'); ->willReturn('SELECT * FROM FOO WHERE BAR = ?');
$queryBuilder $queryBuilder
@ -1315,11 +1321,11 @@ class QueryBuilderTest extends \Test\TestCase {
public function testExecuteWithLoggerAndUnnamedArray() { public function testExecuteWithLoggerAndUnnamedArray() {
$queryBuilder = $this->createMock(\Doctrine\DBAL\Query\QueryBuilder::class); $queryBuilder = $this->createMock(\Doctrine\DBAL\Query\QueryBuilder::class);
$queryBuilder $queryBuilder
->expects($this->at(0)) ->expects($this->any())
->method('getParameters') ->method('getParameters')
->willReturn(['Bar']); ->willReturn(['Bar']);
$queryBuilder $queryBuilder
->expects($this->at(1)) ->expects($this->any())
->method('getSQL') ->method('getSQL')
->willReturn('SELECT * FROM FOO WHERE BAR = ?'); ->willReturn('SELECT * FROM FOO WHERE BAR = ?');
$queryBuilder $queryBuilder
@ -1350,11 +1356,11 @@ class QueryBuilderTest extends \Test\TestCase {
public function testExecuteWithLoggerAndNoParams() { public function testExecuteWithLoggerAndNoParams() {
$queryBuilder = $this->createMock(\Doctrine\DBAL\Query\QueryBuilder::class); $queryBuilder = $this->createMock(\Doctrine\DBAL\Query\QueryBuilder::class);
$queryBuilder $queryBuilder
->expects($this->at(0)) ->expects($this->any())
->method('getParameters') ->method('getParameters')
->willReturn([]); ->willReturn([]);
$queryBuilder $queryBuilder
->expects($this->at(1)) ->expects($this->any())
->method('getSQL') ->method('getSQL')
->willReturn('SELECT * FROM FOO WHERE BAR = ?'); ->willReturn('SELECT * FROM FOO WHERE BAR = ?');
$queryBuilder $queryBuilder
@ -1380,4 +1386,74 @@ class QueryBuilderTest extends \Test\TestCase {
$this->invokePrivate($this->queryBuilder, 'queryBuilder', [$queryBuilder]); $this->invokePrivate($this->queryBuilder, 'queryBuilder', [$queryBuilder]);
$this->assertEquals(3, $this->queryBuilder->execute()); $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();
}
} }