From 2cd92d0abbeffd1817c87522f9b633b14e60181a Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Fri, 28 Oct 2016 11:29:02 +0200 Subject: [PATCH 1/4] Fix missing update of session, when it was already used. Signed-off-by: Joas Schilling --- lib/private/Security/CSRF/TokenStorage/SessionStorage.php | 7 +++++++ lib/private/Server.php | 7 +++++-- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/lib/private/Security/CSRF/TokenStorage/SessionStorage.php b/lib/private/Security/CSRF/TokenStorage/SessionStorage.php index cf4cdfa5036..9d2e723a6d3 100644 --- a/lib/private/Security/CSRF/TokenStorage/SessionStorage.php +++ b/lib/private/Security/CSRF/TokenStorage/SessionStorage.php @@ -40,6 +40,13 @@ class SessionStorage { $this->session = $session; } + /** + * @param ISession $session + */ + public function setSession(ISession $session) { + $this->session = $session; + } + /** * Returns the current token or throws an exception if none is found. * diff --git a/lib/private/Server.php b/lib/private/Server.php index 6f25098eb35..dca50c15733 100644 --- a/lib/private/Server.php +++ b/lib/private/Server.php @@ -710,13 +710,15 @@ class Server extends ServerContainer implements IServerContainer { }); $this->registerService('CsrfTokenManager', function (Server $c) { $tokenGenerator = new CsrfTokenGenerator($c->getSecureRandom()); - $sessionStorage = new SessionStorage($c->getSession()); return new CsrfTokenManager( $tokenGenerator, - $sessionStorage + $c->query(SessionStorage::class) ); }); + $this->registerService(SessionStorage::class, function (Server $c) { + return new SessionStorage($c->getSession()); + }); $this->registerService('ContentSecurityPolicyManager', function (Server $c) { return new ContentSecurityPolicyManager(); }); @@ -945,6 +947,7 @@ class Server extends ServerContainer implements IServerContainer { * @param \OCP\ISession $session */ public function setSession(\OCP\ISession $session) { + $this->query(SessionStorage::class)->setSession($session); return $this->query('UserSession')->setSession($session); } From cd13f50a3f1fbcae757672b4a471d8fbcf959b08 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Fri, 28 Oct 2016 11:29:38 +0200 Subject: [PATCH 2/4] Log the queries of the QueryBuilder as well Signed-off-by: Joas Schilling --- lib/private/DB/QueryBuilder/QueryBuilder.php | 23 ++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/lib/private/DB/QueryBuilder/QueryBuilder.php b/lib/private/DB/QueryBuilder/QueryBuilder.php index eac77dd98fd..aefc84f9c9a 100644 --- a/lib/private/DB/QueryBuilder/QueryBuilder.php +++ b/lib/private/DB/QueryBuilder/QueryBuilder.php @@ -139,6 +139,29 @@ class QueryBuilder implements IQueryBuilder { * @return \Doctrine\DBAL\Driver\Statement|int */ public function execute() { + if (\OC::$server->getConfig()->getSystemValue('log_query', false)) { + $params = []; + foreach ($this->getParameters() as $placeholder => $value) { + if (is_array($value)) { + $params[] = $placeholder . ' => (\'' . implode('\', \'', $value) . '\')'; + } else { + $params[] = $placeholder . ' => \'' . $value . '\''; + } + } + if (empty($params)) { + \OC::$server->getLogger()->debug('DB QueryBuilder: \'{query}\'', [ + 'query' => $this->getSQL(), + 'app' => 'core', + ]); + } else { + \OC::$server->getLogger()->debug('DB QueryBuilder: \'{query}\' with parameters: {params}', [ + 'query' => $this->getSQL(), + 'params' => implode(', ', $params), + 'app' => 'core', + ]); + } + } + return $this->queryBuilder->execute(); } From 2c4035e80679b6ebf8e56d86f88701f3ddb65bec Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Fri, 28 Oct 2016 13:48:58 +0200 Subject: [PATCH 3/4] Inject config and logger Signed-off-by: Joas Schilling --- lib/private/DB/Connection.php | 6 ++++- lib/private/DB/QueryBuilder/QueryBuilder.php | 22 +++++++++++++---- .../lib/DB/QueryBuilder/QueryBuilderTest.php | 24 +++++++++++++++---- tests/lib/Security/CredentialsManagerTest.php | 12 +++++++--- 4 files changed, 51 insertions(+), 13 deletions(-) diff --git a/lib/private/DB/Connection.php b/lib/private/DB/Connection.php index dfe2e86b617..497ff0c8a26 100644 --- a/lib/private/DB/Connection.php +++ b/lib/private/DB/Connection.php @@ -67,7 +67,11 @@ class Connection extends \Doctrine\DBAL\Connection implements IDBConnection { * @return \OCP\DB\QueryBuilder\IQueryBuilder */ public function getQueryBuilder() { - return new QueryBuilder($this); + return new QueryBuilder( + $this, + \OC::$server->getSystemConfig(), + \OC::$server->getLogger() + ); } /** diff --git a/lib/private/DB/QueryBuilder/QueryBuilder.php b/lib/private/DB/QueryBuilder/QueryBuilder.php index aefc84f9c9a..d5dd9cf0366 100644 --- a/lib/private/DB/QueryBuilder/QueryBuilder.php +++ b/lib/private/DB/QueryBuilder/QueryBuilder.php @@ -31,16 +31,24 @@ use OC\DB\QueryBuilder\ExpressionBuilder\ExpressionBuilder; use OC\DB\QueryBuilder\ExpressionBuilder\MySqlExpressionBuilder; use OC\DB\QueryBuilder\ExpressionBuilder\OCIExpressionBuilder; use OC\DB\QueryBuilder\ExpressionBuilder\PgSqlExpressionBuilder; +use OC\SystemConfig; use OCP\DB\QueryBuilder\IQueryBuilder; use OCP\DB\QueryBuilder\IQueryFunction; use OCP\DB\QueryBuilder\IParameter; use OCP\IDBConnection; +use OCP\ILogger; class QueryBuilder implements IQueryBuilder { /** @var \OCP\IDBConnection */ private $connection; + /** @var SystemConfig */ + private $systemConfig; + + /** @var ILogger */ + private $logger; + /** @var \Doctrine\DBAL\Query\QueryBuilder */ private $queryBuilder; @@ -56,10 +64,14 @@ class QueryBuilder implements IQueryBuilder { /** * Initializes a new QueryBuilder. * - * @param \OCP\IDBConnection $connection + * @param IDBConnection $connection + * @param SystemConfig $systemConfig + * @param ILogger $logger */ - public function __construct(IDBConnection $connection) { + public function __construct(IDBConnection $connection, SystemConfig $systemConfig, ILogger $logger) { $this->connection = $connection; + $this->systemConfig = $systemConfig; + $this->logger = $logger; $this->queryBuilder = new \Doctrine\DBAL\Query\QueryBuilder($this->connection); $this->helper = new QuoteHelper(); } @@ -139,7 +151,7 @@ class QueryBuilder implements IQueryBuilder { * @return \Doctrine\DBAL\Driver\Statement|int */ public function execute() { - if (\OC::$server->getConfig()->getSystemValue('log_query', false)) { + if ($this->systemConfig->getValue('log_query', false)) { $params = []; foreach ($this->getParameters() as $placeholder => $value) { if (is_array($value)) { @@ -149,12 +161,12 @@ class QueryBuilder implements IQueryBuilder { } } if (empty($params)) { - \OC::$server->getLogger()->debug('DB QueryBuilder: \'{query}\'', [ + $this->logger->debug('DB QueryBuilder: \'{query}\'', [ 'query' => $this->getSQL(), 'app' => 'core', ]); } else { - \OC::$server->getLogger()->debug('DB QueryBuilder: \'{query}\' with parameters: {params}', [ + $this->logger->debug('DB QueryBuilder: \'{query}\' with parameters: {params}', [ 'query' => $this->getSQL(), 'params' => implode(', ', $params), 'app' => 'core', diff --git a/tests/lib/DB/QueryBuilder/QueryBuilderTest.php b/tests/lib/DB/QueryBuilder/QueryBuilderTest.php index 3c10d25c535..171b44fe92d 100644 --- a/tests/lib/DB/QueryBuilder/QueryBuilderTest.php +++ b/tests/lib/DB/QueryBuilder/QueryBuilderTest.php @@ -25,7 +25,9 @@ use Doctrine\DBAL\Query\Expression\CompositeExpression; use OC\DB\QueryBuilder\Literal; use OC\DB\QueryBuilder\Parameter; use OC\DB\QueryBuilder\QueryBuilder; +use OC\SystemConfig; use OCP\IDBConnection; +use OCP\ILogger; /** * Class QueryBuilderTest @@ -41,11 +43,19 @@ class QueryBuilderTest extends \Test\TestCase { /** @var IDBConnection */ protected $connection; + /** @var SystemConfig|\PHPUnit_Framework_MockObject_MockObject */ + protected $config; + + /** @var ILogger|\PHPUnit_Framework_MockObject_MockObject */ + protected $logger; + protected function setUp() { parent::setUp(); $this->connection = \OC::$server->getDatabaseConnection(); - $this->queryBuilder = new QueryBuilder($this->connection); + $this->config = $this->createMock(SystemConfig::class); + $this->logger = $this->createMock(ILogger::class); + $this->queryBuilder = new QueryBuilder($this->connection, $this->config, $this->logger); } protected function createTestingRows($appId = 'testFirstResult') { @@ -166,7 +176,9 @@ class QueryBuilderTest extends \Test\TestCase { } public function dataSelect() { - $queryBuilder = new QueryBuilder(\OC::$server->getDatabaseConnection()); + $config = $this->createMock(SystemConfig::class); + $logger = $this->createMock(ILogger::class); + $queryBuilder = new QueryBuilder(\OC::$server->getDatabaseConnection(), $config, $logger); return [ // select('column1') [['configvalue'], ['configvalue' => '99']], @@ -232,7 +244,9 @@ class QueryBuilderTest extends \Test\TestCase { } public function dataSelectAlias() { - $queryBuilder = new QueryBuilder(\OC::$server->getDatabaseConnection()); + $config = $this->createMock(SystemConfig::class); + $logger = $this->createMock(ILogger::class); + $queryBuilder = new QueryBuilder(\OC::$server->getDatabaseConnection(), $config, $logger); return [ ['configvalue', 'cv', ['cv' => '99']], [$queryBuilder->expr()->literal('column1'), 'thing', ['thing' => 'column1']], @@ -301,7 +315,9 @@ class QueryBuilderTest extends \Test\TestCase { } public function dataAddSelect() { - $queryBuilder = new QueryBuilder(\OC::$server->getDatabaseConnection()); + $config = $this->createMock(SystemConfig::class); + $logger = $this->createMock(ILogger::class); + $queryBuilder = new QueryBuilder(\OC::$server->getDatabaseConnection(), $config, $logger); return [ // addSelect('column1') [['configvalue'], ['appid' => 'testFirstResult', 'configvalue' => '99']], diff --git a/tests/lib/Security/CredentialsManagerTest.php b/tests/lib/Security/CredentialsManagerTest.php index cffcc02817c..38da26a21a9 100644 --- a/tests/lib/Security/CredentialsManagerTest.php +++ b/tests/lib/Security/CredentialsManagerTest.php @@ -21,6 +21,8 @@ namespace Test\Security; +use OC\SystemConfig; +use OCP\ILogger; use \OCP\Security\ICrypto; use \OCP\IDBConnection; use \OC\Security\CredentialsManager; @@ -45,7 +47,7 @@ class CredentialsManagerTest extends \Test\TestCase { $this->manager = new CredentialsManager($this->crypto, $this->dbConnection); } - private function getQeuryResult($row) { + private function getQueryResult($row) { $result = $this->getMockBuilder('\Doctrine\DBAL\Driver\Statement') ->disableOriginalConstructor() ->getMock(); @@ -87,12 +89,16 @@ class CredentialsManagerTest extends \Test\TestCase { ->willReturn(json_encode('bar')); $qb = $this->getMockBuilder('\OC\DB\QueryBuilder\QueryBuilder') - ->setConstructorArgs([$this->dbConnection]) + ->setConstructorArgs([ + $this->dbConnection, + $this->createMock(SystemConfig::class), + $this->createMock(ILogger::class), + ]) ->setMethods(['execute']) ->getMock(); $qb->expects($this->once()) ->method('execute') - ->willReturn($this->getQeuryResult(['credentials' => 'baz'])); + ->willReturn($this->getQueryResult(['credentials' => 'baz'])); $this->dbConnection->expects($this->once()) ->method('getQueryBuilder') From a8b7df9cc19991bdb936539950400760dfafd29a Mon Sep 17 00:00:00 2001 From: Lukas Reschke Date: Wed, 2 Nov 2016 21:10:51 +0100 Subject: [PATCH 4/4] Add tests Signed-off-by: Lukas Reschke --- .../lib/DB/QueryBuilder/QueryBuilderTest.php | 126 ++++++++++++++++++ .../CSRF/TokenStorage/SessionStorageTest.php | 13 ++ 2 files changed, 139 insertions(+) diff --git a/tests/lib/DB/QueryBuilder/QueryBuilderTest.php b/tests/lib/DB/QueryBuilder/QueryBuilderTest.php index 171b44fe92d..22808f586ef 100644 --- a/tests/lib/DB/QueryBuilder/QueryBuilderTest.php +++ b/tests/lib/DB/QueryBuilder/QueryBuilderTest.php @@ -1216,4 +1216,130 @@ class QueryBuilderTest extends \Test\TestCase { $this->queryBuilder->getColumnName($column, $prefix) ); } + + public function testExecuteWithoutLogger() { + $queryBuilder = $this->createMock(\Doctrine\DBAL\Query\QueryBuilder::class); + $queryBuilder + ->expects($this->once()) + ->method('execute') + ->willReturn(3); + $this->logger + ->expects($this->never()) + ->method('debug'); + $this->config + ->expects($this->once()) + ->method('getValue') + ->with('log_query', false) + ->willReturn(false); + + $this->invokePrivate($this->queryBuilder, 'queryBuilder', [$queryBuilder]); + $this->assertEquals(3, $this->queryBuilder->execute()); + } + + public function testExecuteWithLoggerAndNamedArray() { + $queryBuilder = $this->createMock(\Doctrine\DBAL\Query\QueryBuilder::class); + $queryBuilder + ->expects($this->at(0)) + ->method('getParameters') + ->willReturn([ + 'foo' => 'bar', + 'key' => 'value', + ]); + $queryBuilder + ->expects($this->at(1)) + ->method('getSQL') + ->willReturn('SELECT * FROM FOO WHERE BAR = ?'); + $queryBuilder + ->expects($this->once()) + ->method('execute') + ->willReturn(3); + $this->logger + ->expects($this->once()) + ->method('debug') + ->with( + 'DB QueryBuilder: \'{query}\' with parameters: {params}', + [ + 'query' => 'SELECT * FROM FOO WHERE BAR = ?', + 'params' => 'foo => \'bar\', key => \'value\'', + 'app' => 'core', + ] + ); + $this->config + ->expects($this->once()) + ->method('getValue') + ->with('log_query', false) + ->willReturn(true); + + $this->invokePrivate($this->queryBuilder, 'queryBuilder', [$queryBuilder]); + $this->assertEquals(3, $this->queryBuilder->execute()); + } + + public function testExecuteWithLoggerAndUnnamedArray() { + $queryBuilder = $this->createMock(\Doctrine\DBAL\Query\QueryBuilder::class); + $queryBuilder + ->expects($this->at(0)) + ->method('getParameters') + ->willReturn(['Bar']); + $queryBuilder + ->expects($this->at(1)) + ->method('getSQL') + ->willReturn('SELECT * FROM FOO WHERE BAR = ?'); + $queryBuilder + ->expects($this->once()) + ->method('execute') + ->willReturn(3); + $this->logger + ->expects($this->once()) + ->method('debug') + ->with( + 'DB QueryBuilder: \'{query}\' with parameters: {params}', + [ + 'query' => 'SELECT * FROM FOO WHERE BAR = ?', + 'params' => '0 => \'Bar\'', + 'app' => 'core', + ] + ); + $this->config + ->expects($this->once()) + ->method('getValue') + ->with('log_query', false) + ->willReturn(true); + + $this->invokePrivate($this->queryBuilder, 'queryBuilder', [$queryBuilder]); + $this->assertEquals(3, $this->queryBuilder->execute()); + } + + public function testExecuteWithLoggerAndNoParams() { + $queryBuilder = $this->createMock(\Doctrine\DBAL\Query\QueryBuilder::class); + $queryBuilder + ->expects($this->at(0)) + ->method('getParameters') + ->willReturn([]); + $queryBuilder + ->expects($this->at(1)) + ->method('getSQL') + ->willReturn('SELECT * FROM FOO WHERE BAR = ?'); + $queryBuilder + ->expects($this->once()) + ->method('execute') + ->willReturn(3); + $this->logger + ->expects($this->once()) + ->method('debug') + ->with( + 'DB QueryBuilder: \'{query}\'', + [ + 'query' => 'SELECT * FROM FOO WHERE BAR = ?', + 'app' => 'core', + ] + ); + $this->config + ->expects($this->once()) + ->method('getValue') + ->with('log_query', false) + ->willReturn(true); + + $this->invokePrivate($this->queryBuilder, 'queryBuilder', [$queryBuilder]); + $this->assertEquals(3, $this->queryBuilder->execute()); + } } diff --git a/tests/lib/Security/CSRF/TokenStorage/SessionStorageTest.php b/tests/lib/Security/CSRF/TokenStorage/SessionStorageTest.php index 550fa49e1b2..d1e76684507 100644 --- a/tests/lib/Security/CSRF/TokenStorage/SessionStorageTest.php +++ b/tests/lib/Security/CSRF/TokenStorage/SessionStorageTest.php @@ -21,6 +21,8 @@ namespace Test\Security\CSRF\TokenStorage; +use OCP\ISession; + class SessionStorageTest extends \Test\TestCase { /** @var \OCP\ISession */ private $session; @@ -106,4 +108,15 @@ class SessionStorageTest extends \Test\TestCase { ->willReturn(false); $this->assertSame(false, $this->sessionStorage->hasToken()); } + + public function testSetSession() { + $session = $this->createMock(ISession::class); + $session + ->expects($this->once()) + ->method('get') + ->with('requesttoken') + ->willReturn('MyToken'); + $this->sessionStorage->setSession($session); + $this->assertSame('MyToken', $this->sessionStorage->getToken()); + } }