From f2c220adfc1487c01fffa896be7759c6abfc74b1 Mon Sep 17 00:00:00 2001 From: RazeSoldier Date: Sun, 2 Jun 2019 00:15:34 +0800 Subject: [PATCH] Fix installation failure due to unexpected dbpath under CLI installation 1. Make sure `wgSQLiteDataDir` is not empty. 2. Fix a logic bug In the current logic, in web environment, SqliteInstaller::dataDirOKmaybeCreate() will be executed twice, first in the DBConnect page [1] (with $create=true), and the second time in Installer::performInstallation() [2] (with $create=false), this is a sanity check. But in cli environment, SqliteInstaller::dataDirOKmaybeCreate() will be only executed once, called by Installer::performInstallation() (with $create=false). So the Cli installer will abort because the data directory is checked without the behavior of creating the directory. In this case, I split dataDirOKmaybeCreate() into checkDataDir() and createDataDir() according to its responsibility. And for web installation, we just check the directory on DBConnect page instead of creating it and then actually creating it in setupDatabase(). 3. Add a unit test for SqliteInstaller::dataDirOKmaybeCreate [1] DBConnect page call SqliteInstaller::submitConnectForm(), ::submitConnectForm() call ::dataDirOKmaybeCreate() [2] Installer::performInstallation() call SqliteInstaller::setupDatabase(), ::setupDatabase() call ::dataDirOKmaybeCreate() Bug: T217855 Change-Id: I139036b265716e9898fb76ba907c194f005ea318 --- includes/installer/SqliteInstaller.php | 81 +++++++++++-------- .../installer/SqliteInstallerTest.php | 68 ++++++++++++++++ 2 files changed, 115 insertions(+), 34 deletions(-) create mode 100644 tests/phpunit/unit/includes/installer/SqliteInstallerTest.php diff --git a/includes/installer/SqliteInstaller.php b/includes/installer/SqliteInstaller.php index cf91ccd9fc..d012f1b963 100644 --- a/includes/installer/SqliteInstaller.php +++ b/includes/installer/SqliteInstaller.php @@ -71,16 +71,19 @@ class SqliteInstaller extends DatabaseInstaller { } public function getGlobalDefaults() { + global $IP; $defaults = parent::getGlobalDefaults(); - if ( isset( $_SERVER['DOCUMENT_ROOT'] ) ) { - $path = str_replace( - [ '/', '\\' ], - DIRECTORY_SEPARATOR, - dirname( $_SERVER['DOCUMENT_ROOT'] ) . '/data' - ); - - $defaults['wgSQLiteDataDir'] = $path; + if ( !empty( $_SERVER['DOCUMENT_ROOT'] ) ) { + $path = dirname( $_SERVER['DOCUMENT_ROOT'] ); + } else { + // We use $IP when unable to get $_SERVER['DOCUMENT_ROOT'] + $path = $IP; } + $defaults['wgSQLiteDataDir'] = str_replace( + [ '/', '\\' ], + DIRECTORY_SEPARATOR, + $path . '/data' + ); return $defaults; } @@ -122,7 +125,7 @@ class SqliteInstaller extends DatabaseInstaller { # Try realpath() if the directory already exists $dir = self::realpath( $this->getVar( 'wgSQLiteDataDir' ) ); - $result = self::dataDirOKmaybeCreate( $dir, true /* create? */ ); + $result = self::checkDataDir( $dir ); if ( $result->isOK() ) { # Try expanding again in case we've just created it $dir = self::realpath( $dir ); @@ -135,12 +138,17 @@ class SqliteInstaller extends DatabaseInstaller { } /** - * @param string $dir - * @param bool $create - * @return Status + * Check if the data directory is writable or can be created + * @param string $dir Path to the data directory + * @return Status Return fatal Status if $dir un-writable or no permission to create a directory */ - private static function dataDirOKmaybeCreate( $dir, $create = false ) { - if ( !is_dir( $dir ) ) { + private static function checkDataDir( $dir ) : Status { + if ( is_dir( $dir ) ) { + if ( !is_readable( $dir ) ) { + return Status::newFatal( 'config-sqlite-dir-unwritable', $dir ); + } + } else { + // Check the parent directory if $dir not exists if ( !is_writable( dirname( $dir ) ) ) { $webserverGroup = Installer::maybeGetWebserverPrimaryGroup(); if ( $webserverGroup !== null ) { @@ -156,25 +164,25 @@ class SqliteInstaller extends DatabaseInstaller { ); } } + } + return Status::newGood(); + } - # Called early on in the installer, later we just want to sanity check - # if it's still writable - if ( $create ) { - Wikimedia\suppressWarnings(); - $ok = wfMkdirParents( $dir, 0700, __METHOD__ ); - Wikimedia\restoreWarnings(); - if ( !$ok ) { - return Status::newFatal( 'config-sqlite-mkdir-error', $dir ); - } - # Put a .htaccess file in in case the user didn't take our advice - file_put_contents( "$dir/.htaccess", "Deny from all\n" ); + /** + * @param string $dir Path to the data directory + * @return Status Return good Status if without error + */ + private static function createDataDir( $dir ) : Status { + if ( !is_dir( $dir ) ) { + Wikimedia\suppressWarnings(); + $ok = wfMkdirParents( $dir, 0700, __METHOD__ ); + Wikimedia\restoreWarnings(); + if ( !$ok ) { + return Status::newFatal( 'config-sqlite-mkdir-error', $dir ); } } - if ( !is_writable( $dir ) ) { - return Status::newFatal( 'config-sqlite-dir-unwritable', $dir ); - } - - # We haven't blown up yet, fall through + # Put a .htaccess file in in case the user didn't take our advice + file_put_contents( "$dir/.htaccess", "Deny from all\n" ); return Status::newGood(); } @@ -217,10 +225,15 @@ class SqliteInstaller extends DatabaseInstaller { public function setupDatabase() { $dir = $this->getVar( 'wgSQLiteDataDir' ); - # Sanity check. We checked this before but maybe someone deleted the - # data dir between then and now - $dir_status = self::dataDirOKmaybeCreate( $dir, false /* create? */ ); - if ( !$dir_status->isOK() ) { + # Sanity check (Only available in web installation). We checked this before but maybe someone + # deleted the data dir between then and now + $dir_status = self::checkDataDir( $dir ); + if ( $dir_status->isGood() ) { + $res = self::createDataDir( $dir ); + if ( !$res->isGood() ) { + return $res; + } + } else { return $dir_status; } diff --git a/tests/phpunit/unit/includes/installer/SqliteInstallerTest.php b/tests/phpunit/unit/includes/installer/SqliteInstallerTest.php new file mode 100644 index 0000000000..19a2973a50 --- /dev/null +++ b/tests/phpunit/unit/includes/installer/SqliteInstallerTest.php @@ -0,0 +1,68 @@ +setAccessible( true ); + + # Test 1: Should return fatal Status if $dir exist and it un-writable + if ( ( isset( $_SERVER['USER'] ) && $_SERVER['USER'] !== 'root' ) && !wfIsWindows() ) { + // We can't simulate this environment under Windows or login as root + $dir = sys_get_temp_dir() . '/' . uniqid( 'MediaWikiTest' ); + mkdir( $dir, 0000 ); + /** @var Status $status */ + $status = $method->invoke( null, $dir ); + $this->assertFalse( $status->isGood() ); + $this->assertSame( 'config-sqlite-dir-unwritable', $status->getErrors()[0]['message'] ); + rmdir( $dir ); + } + + # Test 2: Should return fatal Status if $dir not exist and it parent also not exist + $dir = sys_get_temp_dir() . '/' . uniqid( 'MediaWikiTest' ) . '/' . uniqid( 'MediaWikiTest' ); + $status = $method->invoke( null, $dir ); + $this->assertFalse( $status->isGood() ); + + # Test 3: Should return good Status if $dir not exist and it parent writable + $dir = sys_get_temp_dir() . '/' . uniqid( 'MediaWikiTest' ); + /** @var Status $status */ + $status = $method->invoke( null, $dir ); + $this->assertTrue( $status->isGood() ); + } + + /** + * @covers SqliteInstaller::createDataDir + */ + public function testCreateDataDir() { + $method = new ReflectionMethod( SqliteInstaller::class, 'createDataDir' ); + $method->setAccessible( true ); + + # Test 1: Should return fatal Status if $dir not exist and it parent un-writable + if ( ( isset( $_SERVER['USER'] ) && $_SERVER['USER'] !== 'root' ) && !wfIsWindows() ) { + // We can't simulate this environment under Windows or login as root + $random = uniqid( 'MediaWikiTest' ); + $dir = sys_get_temp_dir() . '/' . $random . '/' . uniqid( 'MediaWikiTest' ); + mkdir( sys_get_temp_dir() . "/$random", 0000 ); + /** @var Status $status */ + $status = $method->invoke( null, $dir ); + $this->assertFalse( $status->isGood() ); + $this->assertSame( 'config-sqlite-mkdir-error', $status->getErrors()[0]['message'] ); + rmdir( sys_get_temp_dir() . "/$random" ); + } + + # Test 2: Test .htaccess content after created successfully + $dir = sys_get_temp_dir() . '/' . uniqid( 'MediaWikiTest' ); + $status = $method->invoke( null, $dir ); + $this->assertTrue( $status->isGood() ); + $this->assertSame( "Deny from all\n", file_get_contents( "$dir/.htaccess" ) ); + unlink( "$dir/.htaccess" ); + rmdir( $dir ); + } +} -- 2.20.1