From 8a79d314cf544cf2ca261cbac7ea07570e9ed8e5 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Tue, 12 Jul 2016 13:11:44 +0200 Subject: [PATCH 1/4] Remove duplicate database connect logic in mysql setup --- lib/private/AllConfig.php | 4 +++ lib/private/Setup/AbstractDatabase.php | 22 +++++++++++++++- lib/private/Setup/MySQL.php | 35 -------------------------- 3 files changed, 25 insertions(+), 36 deletions(-) diff --git a/lib/private/AllConfig.php b/lib/private/AllConfig.php index c8b2009fcc..b50fc0f42c 100644 --- a/lib/private/AllConfig.php +++ b/lib/private/AllConfig.php @@ -428,4 +428,8 @@ class AllConfig implements \OCP\IConfig { return $userIDs; } + + public function getSystemConfig() { + return $this->systemConfig; + } } diff --git a/lib/private/Setup/AbstractDatabase.php b/lib/private/Setup/AbstractDatabase.php index 08ed741f51..62e9b2e823 100644 --- a/lib/private/Setup/AbstractDatabase.php +++ b/lib/private/Setup/AbstractDatabase.php @@ -23,6 +23,8 @@ */ namespace OC\Setup; +use OC\AllConfig; +use OC\DB\ConnectionFactory; use OCP\IConfig; use OCP\ILogger; use OCP\Security\ISecureRandom; @@ -45,7 +47,7 @@ abstract class AbstractDatabase { protected $dbPort; /** @var string */ protected $tablePrefix; - /** @var IConfig */ + /** @var AllConfig */ protected $config; /** @var ILogger */ protected $logger; @@ -98,6 +100,24 @@ abstract class AbstractDatabase { $this->tablePrefix = $dbTablePrefix; } + /** + * @return \OC\DB\Connection + * @throws \OC\DatabaseSetupException + */ + protected function connect() { + $systemConfig = $this->config->getSystemConfig(); + $cf = new ConnectionFactory(); + $connectionParams = $cf->createConnectionParams($systemConfig); + // we don't save username/password to the config immediately so this might not be set + if (!$connectionParams['user']) { + $connectionParams['user'] = $this->dbUser; + } + if (!$connectionParams['password']) { + $connectionParams['password'] = $this->dbPassword; + } + return $cf->getConnection($systemConfig->getValue('dbtype', 'sqlite'), $connectionParams); + } + /** * @param string $userName */ diff --git a/lib/private/Setup/MySQL.php b/lib/private/Setup/MySQL.php index 1ff7b278b8..03a1421f42 100644 --- a/lib/private/Setup/MySQL.php +++ b/lib/private/Setup/MySQL.php @@ -87,41 +87,6 @@ class MySQL extends AbstractDatabase { $connection->executeUpdate($query); } - /** - * @return \OC\DB\Connection - * @throws \OC\DatabaseSetupException - */ - private function connect() { - - $connectionParams = array( - 'host' => $this->dbHost, - 'user' => $this->dbUser, - 'password' => $this->dbPassword, - 'tablePrefix' => $this->tablePrefix, - ); - - // adding port support through installer - if(!empty($this->dbPort)) { - if (ctype_digit($this->dbPort)) { - $connectionParams['port'] = $this->dbPort; - } else { - $connectionParams['unix_socket'] = $this->dbPort; - } - } else if (strpos($this->dbHost, ':')) { - // Host variable may carry a port or socket. - list($host, $portOrSocket) = explode(':', $this->dbHost, 2); - if (ctype_digit($portOrSocket)) { - $connectionParams['port'] = $portOrSocket; - } else { - $connectionParams['unix_socket'] = $portOrSocket; - } - $connectionParams['host'] = $host; - } - - $cf = new ConnectionFactory(); - return $cf->getConnection('mysql', $connectionParams); - } - /** * @param $username * @param IDBConnection $connection From 7ffda5d10fa8831f65e42b327bbb56f847b68888 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Tue, 12 Jul 2016 13:50:54 +0200 Subject: [PATCH 2/4] use pdo for postgres setup --- lib/private/Setup/AbstractDatabase.php | 5 +- lib/private/Setup/PostgreSQL.php | 187 +++++++++++-------------- 2 files changed, 86 insertions(+), 106 deletions(-) diff --git a/lib/private/Setup/AbstractDatabase.php b/lib/private/Setup/AbstractDatabase.php index 62e9b2e823..8dee96b1ba 100644 --- a/lib/private/Setup/AbstractDatabase.php +++ b/lib/private/Setup/AbstractDatabase.php @@ -101,10 +101,10 @@ abstract class AbstractDatabase { } /** + * @param array $configOverwrite * @return \OC\DB\Connection - * @throws \OC\DatabaseSetupException */ - protected function connect() { + protected function connect(array $configOverwrite = []) { $systemConfig = $this->config->getSystemConfig(); $cf = new ConnectionFactory(); $connectionParams = $cf->createConnectionParams($systemConfig); @@ -115,6 +115,7 @@ abstract class AbstractDatabase { if (!$connectionParams['password']) { $connectionParams['password'] = $this->dbPassword; } + $connectionParams = array_merge($connectionParams, $configOverwrite); return $cf->getConnection($systemConfig->getValue('dbtype', 'sqlite'), $connectionParams); } diff --git a/lib/private/Setup/PostgreSQL.php b/lib/private/Setup/PostgreSQL.php index 464d1e02e2..30ca88f53a 100644 --- a/lib/private/Setup/PostgreSQL.php +++ b/lib/private/Setup/PostgreSQL.php @@ -26,42 +26,34 @@ */ namespace OC\Setup; +use OC\DatabaseException; +use OC\DB\QueryBuilder\Literal; +use OCP\IDBConnection; + class PostgreSQL extends AbstractDatabase { public $dbprettyname = 'PostgreSQL'; public function setupDatabase($username) { - $e_host = addslashes($this->dbHost); - $e_user = addslashes($this->dbUser); - $e_password = addslashes($this->dbPassword); - - // adding port support through installer - if(!empty($this->dbPort)) { - // casting to int to avoid malicious input - $port = (int)$this->dbPort; - } else if(strpos($e_host, ':')) { - list($e_host, $port)=explode(':', $e_host, 2); - } else { - $port=false; - } - - //check if the database user has admin rights - $connection_string = "host='$e_host' dbname=postgres user='$e_user' port='$port' password='$e_password'"; - $connection = @pg_connect($connection_string); - if(!$connection) { - // Try if we can connect to the DB with the specified name - $e_dbname = addslashes($this->dbName); - $connection_string = "host='$e_host' dbname='$e_dbname' user='$e_user' port='$port' password='$e_password'"; - $connection = @pg_connect($connection_string); - - if(!$connection) - throw new \OC\DatabaseSetupException($this->trans->t('PostgreSQL connection failed'), - $this->trans->t('Please check your connection details.')); - } - $e_user = pg_escape_string($this->dbUser); + $connection = $this->connect([ + 'dbname' => 'postgres' + ]); //check for roles creation rights in postgresql - $query="SELECT 1 FROM pg_roles WHERE rolcreaterole=TRUE AND rolname='$e_user'"; - $result = pg_query($connection, $query); - if($result and pg_num_rows($result) > 0) { + $builder = $connection->getQueryBuilder(); + $builder->automaticTablePrefix(false); + $query = $builder + ->select('rolname') + ->from('pg_roles') + ->where($builder->expr()->eq('rolcreaterole', new Literal('TRUE'))) + ->andWhere($builder->expr()->eq('rolname', $builder->createNamedParameter($this->dbUser))); + + try { + $result = $query->execute(); + $canCreateRoles = $result->rowCount() > 0; + } catch (DatabaseException $e) { + $canCreateRoles = false; + } + + if($canCreateRoles) { //use the admin login data for the new database user //add prefix to the postgresql user name to prevent collisions @@ -72,7 +64,7 @@ class PostgreSQL extends AbstractDatabase { $this->createDBUser($connection); } - $systemConfig = \OC::$server->getSystemConfig(); + $systemConfig = $this->config->getSystemConfig(); $systemConfig->setValues([ 'dbuser' => $this->dbUser, 'dbpassword' => $this->dbPassword, @@ -80,98 +72,85 @@ class PostgreSQL extends AbstractDatabase { //create the database $this->createDatabase($connection); + $query = $connection->prepare("select count(*) FROM pg_class WHERE relname=? limit 1"); + $query->execute([$this->tablePrefix . "users"]); + $tablesSetup = $query->fetchColumn() > 0; // the connection to dbname=postgres is not needed anymore - pg_close($connection); + $connection->close(); // connect to the ownCloud database (dbname=$this->dbname) and check if it needs to be filled $this->dbUser = $systemConfig->getValue('dbuser'); $this->dbPassword = $systemConfig->getValue('dbpassword'); - - $e_host = addslashes($this->dbHost); - $e_dbname = addslashes($this->dbName); - $e_user = addslashes($this->dbUser); - $e_password = addslashes($this->dbPassword); - - // Fix database with port connection - if(strpos($e_host, ':')) { - list($e_host, $port)=explode(':', $e_host, 2); - } else { - $port=false; - } - - $connection_string = "host='$e_host' dbname='$e_dbname' user='$e_user' port='$port' password='$e_password'"; - $connection = @pg_connect($connection_string); - if(!$connection) { + $connection = $this->connect(); + try { + $connection->connect(); + } catch (\Exception $e) { + $this->logger->logException($e); throw new \OC\DatabaseSetupException($this->trans->t('PostgreSQL username and/or password not valid'), - $this->trans->t('You need to enter either an existing account or the administrator.')); + $this->trans->t('You need to enter either an existing account or the administrator.')); } - $query = "select count(*) FROM pg_class WHERE relname='".$this->tablePrefix."users' limit 1"; - $result = pg_query($connection, $query); - if($result) { - $row = pg_fetch_row($result); - } - if(!$result or $row[0]==0) { + + + if(!$tablesSetup) { \OC_DB::createDbFromStructure($this->dbDefinitionFile); } } - private function createDatabase($connection) { - //we can't use OC_BD functions here because we need to connect as the administrative user. - $e_name = pg_escape_string($this->dbName); - $e_user = pg_escape_string($this->dbUser); - $query = "select datname from pg_database where datname = '$e_name'"; - $result = pg_query($connection, $query); - if(!$result) { - $entry = $this->trans->t('DB Error: "%s"', array(pg_last_error($connection))) . '
'; - $entry .= $this->trans->t('Offending command was: "%s"', array($query)) . '
'; - \OCP\Util::writeLog('setup.pg', $entry, \OCP\Util::WARN); - } - if(! pg_fetch_row($result)) { + private function createDatabase(IDBConnection $connection) { + if(!$this->databaseExists($connection)) { //The database does not exists... let's create it - $query = "CREATE DATABASE \"$e_name\" OWNER \"$e_user\""; - $result = pg_query($connection, $query); - if(!$result) { - $entry = $this->trans->t('DB Error: "%s"', array(pg_last_error($connection))) . '
'; - $entry .= $this->trans->t('Offending command was: "%s"', array($query)) . '
'; - \OCP\Util::writeLog('setup.pg', $entry, \OCP\Util::WARN); + $query = $connection->prepare("CREATE DATABASE " . addslashes($this->dbName) . " OWNER " . addslashes($this->dbUser)); + try { + $query->execute(); + } catch (DatabaseException $e) { + $this->logger->error('Error while trying to create database'); + $this->logger->logException($e); } - else { - $query = "REVOKE ALL PRIVILEGES ON DATABASE \"$e_name\" FROM PUBLIC"; - pg_query($connection, $query); + } else { + $query = $connection->prepare("REVOKE ALL PRIVILEGES ON DATABASE " . addslashes($this->dbName) . " FROM PUBLIC"); + try { + $query->execute(); + } catch (DatabaseException $e) { + $this->logger->error('Error while trying to restrict database permissions'); + $this->logger->logException($e); } } } - private function createDBUser($connection) { - $e_name = pg_escape_string($this->dbUser); - $e_password = pg_escape_string($this->dbPassword); - $query = "select * from pg_roles where rolname='$e_name';"; - $result = pg_query($connection, $query); - if(!$result) { - $entry = $this->trans->t('DB Error: "%s"', array(pg_last_error($connection))) . '
'; - $entry .= $this->trans->t('Offending command was: "%s"', array($query)) . '
'; - \OCP\Util::writeLog('setup.pg', $entry, \OCP\Util::WARN); - } + private function userExists(IDBConnection $connection) { + $builder = $connection->getQueryBuilder(); + $builder->automaticTablePrefix(false); + $query = $builder->select('*') + ->from('pg_roles') + ->where($builder->expr()->eq('rolname', $builder->createNamedParameter($this->dbUser))); + $result = $query->execute(); + return $result->rowCount() > 0; + } - if(! pg_fetch_row($result)) { - //user does not exists let's create it :) - $query = "CREATE USER \"$e_name\" CREATEDB PASSWORD '$e_password';"; - $result = pg_query($connection, $query); - if(!$result) { - $entry = $this->trans->t('DB Error: "%s"', array(pg_last_error($connection))) . '
'; - $entry .= $this->trans->t('Offending command was: "%s"', array($query)) . '
'; - \OCP\Util::writeLog('setup.pg', $entry, \OCP\Util::WARN); - } - } - else { // change password of the existing role - $query = "ALTER ROLE \"$e_name\" WITH PASSWORD '$e_password';"; - $result = pg_query($connection, $query); - if(!$result) { - $entry = $this->trans->t('DB Error: "%s"', array(pg_last_error($connection))) . '
'; - $entry .= $this->trans->t('Offending command was: "%s"', array($query)) . '
'; - \OCP\Util::writeLog('setup.pg', $entry, \OCP\Util::WARN); + private function databaseExists(IDBConnection $connection) { + $builder = $connection->getQueryBuilder(); + $builder->automaticTablePrefix(false); + $query = $builder->select('datname') + ->from('pg_database') + ->where($builder->expr()->eq('datname', $builder->createNamedParameter($this->dbName))); + $result = $query->execute(); + return $result->rowCount() > 0; + } + + private function createDBUser(IDBConnection $connection) { + try { + if ($this->userExists($connection, $this->dbUser)) { + // change the password + $query = $connection->prepare("ALTER ROLE " . addslashes($this->dbUser) . " CREATEDB WITH PASSWORD " . addslashes($this->dbPassword)); + } else { + // create the user + $query = $connection->prepare("CREATE USER " . addslashes($this->dbUser) . " CREATEDB PASSWORD " . addslashes($this->dbPassword)); } + $query->execute(); + } catch (DatabaseException $e) { + $this->logger->error('Error while trying to create database user'); + $this->logger->logException($e); } } } From e5d7612a19577995ee01f3271898ab2a2005dd43 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Tue, 12 Jul 2016 14:37:03 +0200 Subject: [PATCH 3/4] dont check for pgsql extension during setup --- lib/private/Setup.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/private/Setup.php b/lib/private/Setup.php index f1454805a0..c78654de95 100644 --- a/lib/private/Setup.php +++ b/lib/private/Setup.php @@ -134,8 +134,8 @@ class Setup { 'name' => 'MySQL/MariaDB' ), 'pgsql' => array( - 'type' => 'function', - 'call' => 'pg_connect', + 'type' => 'pdo', + 'call' => 'pgsql', 'name' => 'PostgreSQL' ), 'oci' => array( From b288c6796aedce787493d24f67d1f39c67e3764d Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Wed, 13 Jul 2016 12:23:44 +0200 Subject: [PATCH 4/4] fix test --- tests/lib/SetupTest.php | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/tests/lib/SetupTest.php b/tests/lib/SetupTest.php index e2723efd76..c6e219f402 100644 --- a/tests/lib/SetupTest.php +++ b/tests/lib/SetupTest.php @@ -57,7 +57,7 @@ class SetupTest extends \Test\TestCase { ->method('is_callable') ->will($this->returnValue(false)); $this->setupClass - ->expects($this->once()) + ->expects($this->any()) ->method('getAvailableDbDriversForPdo') ->will($this->returnValue([])); $result = $this->setupClass->getSupportedDatabases(); @@ -76,15 +76,15 @@ class SetupTest extends \Test\TestCase { array('sqlite', 'mysql', 'oci', 'pgsql') )); $this->setupClass - ->expects($this->once()) + ->expects($this->any()) ->method('class_exists') ->will($this->returnValue(false)); $this->setupClass - ->expects($this->exactly(2)) + ->expects($this->any()) ->method('is_callable') ->will($this->returnValue(false)); $this->setupClass - ->expects($this->once()) + ->expects($this->any()) ->method('getAvailableDbDriversForPdo') ->will($this->returnValue([])); $result = $this->setupClass->getSupportedDatabases(); @@ -100,17 +100,17 @@ class SetupTest extends \Test\TestCase { array('sqlite', 'mysql', 'pgsql', 'oci') )); $this->setupClass - ->expects($this->once()) + ->expects($this->any()) ->method('class_exists') ->will($this->returnValue(true)); $this->setupClass - ->expects($this->exactly(2)) + ->expects($this->any()) ->method('is_callable') ->will($this->returnValue(true)); $this->setupClass - ->expects($this->once()) + ->expects($this->any()) ->method('getAvailableDbDriversForPdo') - ->will($this->returnValue(['mysql'])); + ->will($this->returnValue(['mysql', 'pgsql'])); $result = $this->setupClass->getSupportedDatabases(); $expectedResult = array( 'sqlite' => 'SQLite',