Merge pull request #10934 from owncloud/datadir-write-setup

Don't complain about non-writable datadirs before we're installed

core/setup/controller.php

@@ -116,16 +116,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()) {

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) {

lib/private/setup.php

@@ -37,18 +37,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'];

lib/private/util.php

@@ -409,14 +409,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);
 		}
@@ -456,7 +457,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())
@@ -471,25 +472,27 @@ class OC_Util {
 			}
 		}
 		// Create root dir.
-		if (!is_dir($CONFIG_DATADIRECTORY)) {
-			$success = @mkdir($CONFIG_DATADIRECTORY);
-			if ($success) {
-				$errors = array_merge($errors, self::checkDataDirectoryPermissions($CONFIG_DATADIRECTORY));
-			} else {
+		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 '
+							. '<a href="%s" target="_blank">giving the webserver write access to the root directory</a>.',
+							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 '
-						. '<a href="%s" target="_blank">giving the webserver write access to the root directory</a>.',
-						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()) {
@@ -1310,11 +1313,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;

lib/public/util.php

@@ -529,6 +529,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());
 	}
 }

tests/lib/utilcheckserver.php

@@ -13,10 +13,30 @@ 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;
+		$systemOptions['appstoreenabled'] = false; //it's likely that there is no app folder we can write in
+		$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', '');
+		\OC::$server->getSession()->set('checkServer_succeeded', false);
 	}
 
 	public function tearDown() {
@@ -28,7 +48,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 +63,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 +82,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 +108,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 +116,46 @@ 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);
+	}
+
+	/**
+	 * 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);
+	}
 }