diff --git a/src/Application/Install/Services/Installer.php b/src/Application/Install/Services/Installer.php index 3f5a738f3..2f984cfc3 100644 --- a/src/Application/Install/Services/Installer.php +++ b/src/Application/Install/Services/Installer.php @@ -229,12 +229,18 @@ private function install(): void $configData->setDbPass($dbPass); } + $createdDatabase = false; + try { // Inside the try: setupDbUser() already created the runtime user, so a // failure saving the config must roll it back too $this->config->save($configData, false); $this->databaseSetup->createDatabase($dbUser); + + // Only now may a rollback drop it. Until this line the database is either absent or + // somebody else's, and the rollback below used to drop it either way. + $createdDatabase = true; $this->databaseSetup->createDBStructure(); $this->databaseSetup->checkConnection(); @@ -264,7 +270,7 @@ private function install(): void // back over the admin connection $this->databaseConnectionData->refreshFromInstallData($this->installData); - $this->databaseSetup->rollback($dbUser); + $this->databaseSetup->rollback($dbUser, $createdDatabase); throw $e instanceof SPException ? $e diff --git a/src/Application/Install/Services/MysqlSetup.php b/src/Application/Install/Services/MysqlSetup.php index b63f56b02..33e74aeb8 100644 --- a/src/Application/Install/Services/MysqlSetup.php +++ b/src/Application/Install/Services/MysqlSetup.php @@ -345,7 +345,7 @@ public function checkDatabaseExists(): bool * Best-effort: a rollback failure must never mask the error that triggered * it, and one failed statement must not stop the remaining cleanup. */ - public function rollback(?string $dbUser = null): void + public function rollback(?string $dbUser = null, bool $createdDatabase = false): void { try { $dbc = $this->dbStorage->getConnectionSimple(); @@ -385,13 +385,32 @@ public function rollback(?string $dbUser = null): void $this->execBestEffort($dbc, 'SET FOREIGN_KEY_CHECKS = 1'); } else { - $this->execBestEffort( - $dbc, - sprintf( - 'DROP DATABASE IF EXISTS `%s`', - $this->installData->getDbName() - ) - ); + // Only the database this run created, the way the user below is only dropped when + // this run created that. + // + // The drop used to be unconditional, and nothing established whose database it was. + // `install/install` is unauthenticated by necessity, `checkDatabaseAvailability()` + // runs before anything is created and `createDatabase()` well after it, with no lock + // between them — so two requests (an impatient double-click is enough) both pass the + // availability check, the second fails on `CREATE SCHEMA` because the name is now + // taken, and its rollback dropped the database the first had just finished installing + // into. `config.xml` already said `installed=1`, so the instance claimed to be + // installed with no schema behind it, and every later request went to + // `error/databaseError`. + // + // The comment above `checkDatabaseAvailability()` in `Installer::install()` shows the + // hazard was understood — "a failure here must not trigger a rollback, which could + // otherwise touch pre-existing data". This is that same rule, applied where the + // rollback happens rather than where the check does. + if ($createdDatabase) { + $this->execBestEffort( + $dbc, + sprintf( + 'DROP DATABASE IF EXISTS `%s`', + $this->installData->getDbName() + ) + ); + } if ($dbUser) { $this->execBestEffort( diff --git a/src/Domain/Install/Services/DatabaseSetupService.php b/src/Domain/Install/Services/DatabaseSetupService.php index 49e76f490..7dca80dfa 100644 --- a/src/Domain/Install/Services/DatabaseSetupService.php +++ b/src/Domain/Install/Services/DatabaseSetupService.php @@ -73,8 +73,9 @@ public function checkConnection(): void; /** * Roll back the installation in case of failure. * - * Removes the sysPass database and user. Best-effort: it must never - * throw, so it cannot mask the error that triggered it. + * Removes what *this* run created, and only that: the user when `$dbUser` names one, and the + * database when `$createdDatabase` says this run created it. Best-effort: it must never throw, + * so it cannot mask the error that triggered it. */ - public function rollback(?string $dbUser = null): void; + public function rollback(?string $dbUser = null, bool $createdDatabase = false): void; } diff --git a/tests/Unit/Application/Install/Services/InstallerTest.php b/tests/Unit/Application/Install/Services/InstallerTest.php index 2959986ee..4e425b5ba 100644 --- a/tests/Unit/Application/Install/Services/InstallerTest.php +++ b/tests/Unit/Application/Install/Services/InstallerTest.php @@ -395,6 +395,76 @@ public function testHostingModeIsUsed(): void $this->assertEquals($params->getDbAdminPass(), $configData->getDbPass()); } + /** + * A rollback drops the database only when this run was the one that created it. + * + * `install/install` is unauthenticated by necessity, and the drop used to be unconditional — + * nothing established whose database it was. `checkDatabaseAvailability()` runs before anything + * is created and `createDatabase()` well after, with no lock between them, so two requests (an + * impatient double-click is enough) both pass the availability check; the second fails on + * `CREATE SCHEMA` because the name is now taken, and its rollback dropped the database the + * first had just finished installing into. `config.xml` already said `installed=1`, so the + * instance claimed to be installed with no schema behind it. + * + * Here the failure happens *before* `createDatabase()` returns, which is exactly the losing + * request's shape. + * + * @throws InvalidArgumentException + * @throws SPException + */ + public function testARollbackBeforeTheDatabaseWasCreatedDropsNoDatabase(): void + { + // Non-hosting on purpose: the unconditional DROP DATABASE was on that branch. The runtime + // user is created before the try block, so the pair has to be there to destructure. + $this->databaseSetup->method('setupDbUser')->willReturn(['sp_user', 'sp_pass']); + + $this->databaseSetup + ->method('createDatabase') + ->willThrowException(SPException::error('Error while creating the DB')); + + $this->databaseSetup + ->expects($this->once()) + ->method('rollback') + ->with(self::anything(), false); + + $params = $this->getInstallData(); + + $installer = $this->getDefaultInstaller(); + + $this->expectException(SPException::class); + + $installer->run($params); + } + + /** + * ...and it does drop it once this run has created it, or a genuinely failed install would + * leave its own half-built schema behind and refuse every retry. + * + * @throws InvalidArgumentException + * @throws SPException + */ + public function testARollbackAfterTheDatabaseWasCreatedDropsIt(): void + { + $this->databaseSetup->method('setupDbUser')->willReturn(['sp_user', 'sp_pass']); + + $this->databaseSetup + ->method('createDBStructure') + ->willThrowException(SPException::error('Error while creating the DB structure')); + + $this->databaseSetup + ->expects($this->once()) + ->method('rollback') + ->with(self::anything(), true); + + $params = $this->getInstallData(); + + $installer = $this->getDefaultInstaller(); + + $this->expectException(SPException::class); + + $installer->run($params); + } + /** * @throws InvalidArgumentException * @throws SPException diff --git a/tests/Unit/Application/Install/Services/MySQLTest.php b/tests/Unit/Application/Install/Services/MySQLTest.php index 3aef42dcb..3db91731c 100644 --- a/tests/Unit/Application/Install/Services/MySQLTest.php +++ b/tests/Unit/Application/Install/Services/MySQLTest.php @@ -554,7 +554,8 @@ public function testRollbackIsSuccessful(): void $this->pdo->method('quote')->willReturnArgument(0); - $this->mysqlService->rollback($this->configData->getDbUser()); + // createdDatabase: this run made it, so the rollback owns it. + $this->mysqlService->rollback($this->configData->getDbUser(), true); } public function testRollbackIsSuccessfulWithSameDnsHost(): void @@ -585,7 +586,8 @@ public function testRollbackIsSuccessfulWithSameDnsHost(): void $this->pdo->method('quote')->willReturnArgument(0); - $this->mysqlService->rollback($this->configData->getDbUser()); + // createdDatabase: this run made it, so the rollback owns it. + $this->mysqlService->rollback($this->configData->getDbUser(), true); } public function testRollbackIsSuccessfulWithHostingMode(): void @@ -619,6 +621,21 @@ public function testRollbackNeverThrows(): void ->willThrowException(new PDOException('test')); // Best-effort: a rollback failure must not mask the error that triggered it + $this->mysqlService->rollback(null, true); + } + + /** + * A rollback that did not create the database does not drop one. + * + * `install/install` is unauthenticated by necessity, and the drop used to be unconditional — + * nothing established whose database it was. Two requests both pass + * `checkDatabaseAvailability()` before either creates anything, and the one that then loses the + * `CREATE SCHEMA` race used to drop the database the winner had just installed into. + */ + public function testRollbackDropsNoDatabaseItDidNotCreate(): void + { + $this->pdo->expects(self::never())->method('exec'); + $this->mysqlService->rollback(); }