From 6fa3280c2a4033e99b867440ae49932390784c48 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Mon, 8 Sep 2014 14:56:11 +0200 Subject: [PATCH 1/4] Inject config into checkserver and cleanup tests --- lib/base.php | 2 +- lib/private/util.php | 18 ++++++------ lib/public/util.php | 2 +- tests/lib/utilcheckserver.php | 54 +++++++++++++++++++++++------------ 4 files changed, 47 insertions(+), 29 deletions(-) diff --git a/lib/base.php b/lib/base.php index e21d0124b32..5870497834b 100644 --- a/lib/base.php +++ b/lib/base.php @@ -531,7 +531,7 @@ class OC { self::checkSSL(); OC_Response::addSecurityHeaders(); - $errors = OC_Util::checkServer(); + $errors = OC_Util::checkServer(\OC::$server->getConfig()); if (count($errors) > 0) { if (self::$CLI) { foreach ($errors as $error) { diff --git a/lib/private/util.php b/lib/private/util.php index c5483c1654b..04f00f42447 100755 --- a/lib/private/util.php +++ b/lib/private/util.php @@ -401,14 +401,15 @@ class OC_Util { /** * check if the current server configuration is suitable for ownCloud * + * @param \OCP\IConfig $config * @return array arrays with error messages and hints */ - public static function checkServer() { + public static function checkServer($config) { $l = \OC::$server->getL10N('lib'); $errors = array(); - $CONFIG_DATADIRECTORY = OC_Config::getValue('datadirectory', OC::$SERVERROOT . '/data'); + $CONFIG_DATADIRECTORY = $config->getSystemValue('datadirectory', OC::$SERVERROOT . '/data'); - if (!self::needUpgrade() && OC_Config::getValue('installed', false)) { + if (!self::needUpgrade($config) && $config->getSystemValue('installed', false)) { // this check needs to be done every time $errors = self::checkDataDirectoryValidity($CONFIG_DATADIRECTORY); } @@ -448,7 +449,7 @@ class OC_Util { } // Check if there is a writable install folder. - if (OC_Config::getValue('appstoreenabled', true)) { + if ($config->getSystemValue('appstoreenabled', true)) { if (OC_App::getInstallPath() === null || !is_writable(OC_App::getInstallPath()) || !is_readable(OC_App::getInstallPath()) @@ -462,7 +463,7 @@ class OC_Util { ); } } - // Create root dir. + if (!is_dir($CONFIG_DATADIRECTORY)) { $success = @mkdir($CONFIG_DATADIRECTORY); if ($success) { @@ -1435,11 +1436,12 @@ class OC_Util { * either when the core version is higher or any app requires * an upgrade. * + * @param \OCP\IConfig $config * @return bool whether the core or any app needs an upgrade */ - public static function needUpgrade() { - if (OC_Config::getValue('installed', false)) { - $installedVersion = OC_Config::getValue('version', '0.0.0'); + public static function needUpgrade($config) { + if ($config->getSystemValue('installed', false)) { + $installedVersion = $config->getSystemValue('version', '0.0.0'); $currentVersion = implode('.', OC_Util::getVersion()); if (version_compare($currentVersion, $installedVersion, '>')) { return true; diff --git a/lib/public/util.php b/lib/public/util.php index 244c11ba2cc..d3f07b5c459 100644 --- a/lib/public/util.php +++ b/lib/public/util.php @@ -545,6 +545,6 @@ class Util { * @return bool true if upgrade is needed, false otherwise */ public static function needUpgrade() { - return \OC_Util::needUpgrade(); + return \OC_Util::needUpgrade(\OC::$server->getConfig()); } } diff --git a/tests/lib/utilcheckserver.php b/tests/lib/utilcheckserver.php index 155d617c4ad..78888bd909d 100644 --- a/tests/lib/utilcheckserver.php +++ b/tests/lib/utilcheckserver.php @@ -13,8 +13,26 @@ class Test_Util_CheckServer extends PHPUnit_Framework_TestCase { private $datadir; + /** + * @param array $systemOptions + * @return \OCP\IConfig | PHPUnit_Framework_MockObject_MockObject + */ + protected function getConfig($systemOptions) { + $systemOptions['datadirectory'] = $this->datadir; + $config = $this->getMockBuilder('\OCP\IConfig') + ->disableOriginalConstructor() + ->getMock(); + + $config->expects($this->any()) + ->method('getSystemValue') + ->will($this->returnCallback(function ($key, $default) use ($systemOptions) { + return isset($systemOptions[$key]) ? $systemOptions[$key] : $default; + })); + return $config; + } + public function setUp() { - $this->datadir = \OC_Config::getValue('datadirectory', \OC::$SERVERROOT . '/data'); + $this->datadir = \OC_Helper::tmpFolder(); file_put_contents($this->datadir . '/.ocdata', ''); } @@ -28,7 +46,9 @@ class Test_Util_CheckServer extends PHPUnit_Framework_TestCase { * Test that checkServer() returns no errors in the regular case. */ public function testCheckServer() { - $result = \OC_Util::checkServer(); + $result = \OC_Util::checkServer($this->getConfig(array( + 'installed' => true + ))); $this->assertEmpty($result); } @@ -41,19 +61,12 @@ class Test_Util_CheckServer extends PHPUnit_Framework_TestCase { // simulate old version that didn't have it unlink($this->datadir . '/.ocdata'); - $session = \OC::$server->getSession(); - $oldInstalled = \OC_Config::getValue('installed', false); - - // simulate that the server isn't setup yet - \OC_Config::setValue('installed', false); - // even though ".ocdata" is missing, the error isn't // triggered to allow setup to run - $result = \OC_Util::checkServer(); + $result = \OC_Util::checkServer($this->getConfig(array( + 'installed' => false + ))); $this->assertEmpty($result); - - // restore config - \OC_Config::setValue('installed', $oldInstalled); } /** @@ -67,20 +80,20 @@ class Test_Util_CheckServer extends PHPUnit_Framework_TestCase { $session = \OC::$server->getSession(); $oldCurrentVersion = $session->get('OC_Version'); - $oldInstallVersion = \OC_Config::getValue('version', '0.0.0'); // upgrade condition to simulate needUpgrade() === true $session->set('OC_Version', array(6, 0, 0, 2)); - \OC_Config::setValue('version', '6.0.0.1'); // even though ".ocdata" is missing, the error isn't // triggered to allow for upgrade - $result = \OC_Util::checkServer(); + $result = \OC_Util::checkServer($this->getConfig(array( + 'installed' => true, + 'version' => '6.0.0.1' + ))); $this->assertEmpty($result); // restore versions $session->set('OC_Version', $oldCurrentVersion); - \OC_Config::setValue('version', $oldInstallVersion); } /** @@ -93,7 +106,7 @@ class Test_Util_CheckServer extends PHPUnit_Framework_TestCase { } /** - * Test that checkDataDirectoryValidity and checkServer + * Test that checkDataDirectoryValidity and checkServer * both return an error when ".ocdata" is missing. */ public function testCheckDataDirValidityWhenFileMissing() { @@ -101,8 +114,11 @@ class Test_Util_CheckServer extends PHPUnit_Framework_TestCase { $result = \OC_Util::checkDataDirectoryValidity($this->datadir); $this->assertEquals(1, count($result)); - $result = \OC_Util::checkServer(); - $this->assertEquals(1, count($result)); + $result = \OC_Util::checkServer($this->getConfig(array( + 'installed' => true, + 'version' => implode('.', OC_Util::getVersion()) + ))); + $this->assertCount(1, $result); } } From 23dd7cb51d392d2d6fb82b55c5f51a6bf6e93733 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Mon, 8 Sep 2014 15:05:57 +0200 Subject: [PATCH 2/4] Don't complain about non-writable datadirs before we're installed --- lib/private/util.php | 36 ++++++++++++++++++---------------- tests/lib/utilcheckserver.php | 37 +++++++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+), 17 deletions(-) diff --git a/lib/private/util.php b/lib/private/util.php index 04f00f42447..a9c4cc70a0e 100755 --- a/lib/private/util.php +++ b/lib/private/util.php @@ -463,26 +463,28 @@ class OC_Util { ); } } - - if (!is_dir($CONFIG_DATADIRECTORY)) { - $success = @mkdir($CONFIG_DATADIRECTORY); - if ($success) { - $errors = array_merge($errors, self::checkDataDirectoryPermissions($CONFIG_DATADIRECTORY)); - } else { + // Create root dir. + if ($config->getSystemValue('installed', false)) { + if (!is_dir($CONFIG_DATADIRECTORY)) { + $success = @mkdir($CONFIG_DATADIRECTORY); + if ($success) { + $errors = array_merge($errors, self::checkDataDirectoryPermissions($CONFIG_DATADIRECTORY)); + } else { + $errors[] = array( + 'error' => $l->t('Cannot create "data" directory (%s)', array($CONFIG_DATADIRECTORY)), + 'hint' => $l->t('This can usually be fixed by ' + . 'giving the webserver write access to the root directory.', + array(OC_Helper::linkToDocs('admin-dir_permissions'))) + ); + } + } else if (!is_writable($CONFIG_DATADIRECTORY) or !is_readable($CONFIG_DATADIRECTORY)) { $errors[] = array( - 'error' => $l->t('Cannot create "data" directory (%s)', array($CONFIG_DATADIRECTORY)), - 'hint' => $l->t('This can usually be fixed by ' - . 'giving the webserver write access to the root directory.', - array(OC_Helper::linkToDocs('admin-dir_permissions'))) + 'error' => 'Data directory (' . $CONFIG_DATADIRECTORY . ') not writable by ownCloud', + 'hint' => $permissionsHint ); + } else { + $errors = array_merge($errors, self::checkDataDirectoryPermissions($CONFIG_DATADIRECTORY)); } - } else if (!is_writable($CONFIG_DATADIRECTORY) or !is_readable($CONFIG_DATADIRECTORY)) { - $errors[] = array( - 'error' => 'Data directory (' . $CONFIG_DATADIRECTORY . ') not writable by ownCloud', - 'hint' => $permissionsHint - ); - } else { - $errors = array_merge($errors, self::checkDataDirectoryPermissions($CONFIG_DATADIRECTORY)); } if (!OC_Util::isSetLocaleWorking()) { diff --git a/tests/lib/utilcheckserver.php b/tests/lib/utilcheckserver.php index 78888bd909d..be5596c1900 100644 --- a/tests/lib/utilcheckserver.php +++ b/tests/lib/utilcheckserver.php @@ -19,6 +19,7 @@ class Test_Util_CheckServer extends PHPUnit_Framework_TestCase { */ protected function getConfig($systemOptions) { $systemOptions['datadirectory'] = $this->datadir; + $systemOptions['appstoreenabled'] = false; //it's likely that there is no app folder we can write in $config = $this->getMockBuilder('\OCP\IConfig') ->disableOriginalConstructor() ->getMock(); @@ -35,6 +36,7 @@ class Test_Util_CheckServer extends PHPUnit_Framework_TestCase { $this->datadir = \OC_Helper::tmpFolder(); file_put_contents($this->datadir . '/.ocdata', ''); + \OC::$server->getSession()->set('checkServer_succeeded', false); } public function tearDown() { @@ -121,4 +123,39 @@ class Test_Util_CheckServer extends PHPUnit_Framework_TestCase { $this->assertCount(1, $result); } + /** + * Tests that no error is given when the datadir is writable + */ + public function testDataDirWritable() { + $result = \OC_Util::checkServer($this->getConfig(array( + 'installed' => true, + 'version' => implode('.', OC_Util::getVersion()) + ))); + $this->assertEmpty($result); + } + + /** + * Tests an error is given when the datadir is not writable + */ + public function testDataDirNotWritable() { + chmod($this->datadir, 0300); + $result = \OC_Util::checkServer($this->getConfig(array( + 'installed' => true, + 'version' => implode('.', OC_Util::getVersion()) + ))); + $this->assertCount(1, $result); + } + + /** + * Tests no error is given when the datadir is not writable during setup + */ + public function testDataDirNotWritableSetup() { + chmod($this->datadir, 0300); + $result = \OC_Util::checkServer($this->getConfig(array( + 'installed' => false, + 'version' => implode('.', OC_Util::getVersion()) + ))); + chmod($this->datadir, 0700); //needed for cleanup + $this->assertEmpty($result); + } } From c8dbdc29d028a56a9fb55ae3737434495f3e0eab Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Thu, 18 Sep 2014 14:15:52 +0200 Subject: [PATCH 3/4] Check for writable datadir during setup --- lib/private/setup.php | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/lib/private/setup.php b/lib/private/setup.php index 95e908d10ec..fa1f5699511 100644 --- a/lib/private/setup.php +++ b/lib/private/setup.php @@ -38,18 +38,28 @@ class OC_Setup { $dbtype = 'sqlite'; } + $username = htmlspecialchars_decode($options['adminlogin']); + $password = htmlspecialchars_decode($options['adminpass']); + $datadir = htmlspecialchars_decode($options['directory']); + $class = self::$dbSetupClasses[$dbtype]; + /** @var \OC\Setup\AbstractDatabase $dbSetup */ $dbSetup = new $class(self::getTrans(), 'db_structure.xml'); $error = array_merge($error, $dbSetup->validate($options)); + // validate the data directory + if ( + (!is_dir($datadir) and !mkdir($datadir)) or + !is_writable($datadir) + ) { + $error[] = $l->t("Can't create or write into the data directory %s", array($datadir)); + } + if(count($error) != 0) { return $error; } //no errors, good - $username = htmlspecialchars_decode($options['adminlogin']); - $password = htmlspecialchars_decode($options['adminpass']); - $datadir = htmlspecialchars_decode($options['directory']); if( isset($options['trusted_domains']) && is_array($options['trusted_domains'])) { $trustedDomains = $options['trusted_domains']; From 6ff29f3874cac3c263c255b2c36cda5b8a1c67f1 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Thu, 18 Sep 2014 14:23:38 +0200 Subject: [PATCH 4/4] Don't test for htaccess if we cant write into the datadir anyway --- core/setup/controller.php | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/core/setup/controller.php b/core/setup/controller.php index 5da94e83ccc..9dcc9acfc64 100644 --- a/core/setup/controller.php +++ b/core/setup/controller.php @@ -119,16 +119,18 @@ class Controller { $errors = array(); - // Protect data directory here, so we can test if the protection is working - \OC_Setup::protectDataDirectory(); - try { - $htaccessWorking = \OC_Util::isHtaccessWorking(); - } catch (\OC\HintException $e) { - $errors[] = array( - 'error' => $e->getMessage(), - 'hint' => $e->getHint() - ); - $htaccessWorking = false; + if (is_dir($datadir) and is_writable($datadir)) { + // Protect data directory here, so we can test if the protection is working + \OC_Setup::protectDataDirectory(); + try { + $htaccessWorking = \OC_Util::isHtaccessWorking(); + } catch (\OC\HintException $e) { + $errors[] = array( + 'error' => $e->getMessage(), + 'hint' => $e->getHint() + ); + $htaccessWorking = false; + } } if (\OC_Util::runningOnMac()) {