diff --git a/app/Http/Controllers/Admin/ServersController.php b/app/Http/Controllers/Admin/ServersController.php index dc2168605..61c0511e9 100644 --- a/app/Http/Controllers/Admin/ServersController.php +++ b/app/Http/Controllers/Admin/ServersController.php @@ -409,7 +409,7 @@ class ServersController extends Controller ['id', '=', $database], ]); - $this->databaseManagementService->delete($database->id); + $this->databaseManagementService->delete($database); return response('', 204); } diff --git a/app/Http/Controllers/Api/Application/Servers/DatabaseController.php b/app/Http/Controllers/Api/Application/Servers/DatabaseController.php index 24c8906aa..936e4acb6 100644 --- a/app/Http/Controllers/Api/Application/Servers/DatabaseController.php +++ b/app/Http/Controllers/Api/Application/Servers/DatabaseController.php @@ -133,7 +133,7 @@ class DatabaseController extends ApplicationApiController */ public function delete(ServerDatabaseWriteRequest $request): Response { - $this->databaseManagementService->delete($request->getModel(Database::class)->id); + $this->databaseManagementService->delete($request->getModel(Database::class)); return response('', 204); } diff --git a/app/Http/Controllers/Api/Client/Servers/DatabaseController.php b/app/Http/Controllers/Api/Client/Servers/DatabaseController.php index 5a3e1e3ac..2eacfb9e2 100644 --- a/app/Http/Controllers/Api/Client/Servers/DatabaseController.php +++ b/app/Http/Controllers/Api/Client/Servers/DatabaseController.php @@ -129,7 +129,7 @@ class DatabaseController extends ClientApiController */ public function delete(DeleteDatabaseRequest $request, Server $server, Database $database): Response { - $this->managementService->delete($database->id); + $this->managementService->delete($database); return Response::create('', Response::HTTP_NO_CONTENT); } diff --git a/app/Services/Databases/DatabaseManagementService.php b/app/Services/Databases/DatabaseManagementService.php index ab415b1ab..4291e9353 100644 --- a/app/Services/Databases/DatabaseManagementService.php +++ b/app/Services/Databases/DatabaseManagementService.php @@ -151,20 +151,19 @@ class DatabaseManagementService /** * Delete a database from the given host server. * - * @param int $id + * @param \Pterodactyl\Models\Database $database * @return bool|null * - * @throws \Pterodactyl\Exceptions\Repository\RecordNotFoundException + * @throws \Exception */ - public function delete($id) + public function delete(Database $database) { - $database = $this->repository->find($id); $this->dynamic->set('dynamic', $database->database_host_id); $this->repository->dropDatabase($database->database); $this->repository->dropUser($database->username, $database->remote); $this->repository->flush(); - return $this->repository->delete($id); + return $database->delete(); } } diff --git a/app/Services/Servers/ServerDeletionService.php b/app/Services/Servers/ServerDeletionService.php index 8d7217769..95585873b 100644 --- a/app/Services/Servers/ServerDeletionService.php +++ b/app/Services/Servers/ServerDeletionService.php @@ -3,11 +3,10 @@ namespace Pterodactyl\Services\Servers; use Exception; -use Psr\Log\LoggerInterface; +use Illuminate\Http\Response; use Pterodactyl\Models\Server; +use Illuminate\Support\Facades\Log; use Illuminate\Database\ConnectionInterface; -use Pterodactyl\Repositories\Eloquent\ServerRepository; -use Pterodactyl\Repositories\Eloquent\DatabaseRepository; use Pterodactyl\Repositories\Wings\DaemonServerRepository; use Pterodactyl\Services\Databases\DatabaseManagementService; use Pterodactyl\Exceptions\Http\Connection\DaemonConnectionException; @@ -29,50 +28,26 @@ class ServerDeletionService */ private $daemonServerRepository; - /** - * @var \Pterodactyl\Repositories\Eloquent\DatabaseRepository - */ - private $databaseRepository; - /** * @var \Pterodactyl\Services\Databases\DatabaseManagementService */ private $databaseManagementService; - /** - * @var \Pterodactyl\Repositories\Eloquent\ServerRepository - */ - private $repository; - - /** - * @var \Psr\Log\LoggerInterface - */ - private $writer; - /** * DeletionService constructor. * * @param \Illuminate\Database\ConnectionInterface $connection * @param \Pterodactyl\Repositories\Wings\DaemonServerRepository $daemonServerRepository - * @param \Pterodactyl\Repositories\Eloquent\DatabaseRepository $databaseRepository * @param \Pterodactyl\Services\Databases\DatabaseManagementService $databaseManagementService - * @param \Pterodactyl\Repositories\Eloquent\ServerRepository $repository - * @param \Psr\Log\LoggerInterface $writer */ public function __construct( ConnectionInterface $connection, DaemonServerRepository $daemonServerRepository, - DatabaseRepository $databaseRepository, - DatabaseManagementService $databaseManagementService, - ServerRepository $repository, - LoggerInterface $writer + DatabaseManagementService $databaseManagementService ) { $this->connection = $connection; $this->daemonServerRepository = $daemonServerRepository; - $this->databaseRepository = $databaseRepository; $this->databaseManagementService = $databaseManagementService; - $this->repository = $repository; - $this->writer = $writer; } /** @@ -101,27 +76,39 @@ class ServerDeletionService try { $this->daemonServerRepository->setServer($server)->delete(); } catch (DaemonConnectionException $exception) { - if ($this->force) { - $this->writer->warning($exception); - } else { + // If there is an error not caused a 404 error and this isn't a forced delete, + // go ahead and bail out. We specifically ignore a 404 since that can be assumed + // to be a safe error, meaning the server doesn't exist at all on Wings so there + // is no reason we need to bail out from that. + if (! $this->force && $exception->getStatusCode() !== Response::HTTP_NOT_FOUND) { throw $exception; } + + Log::warning($exception); } $this->connection->transaction(function () use ($server) { - $this->databaseRepository->setColumns('id')->findWhere([['server_id', '=', $server->id]])->each(function ($item) { + foreach ($server->databases as $database) { try { - $this->databaseManagementService->delete($item->id); + $this->databaseManagementService->delete($database); } catch (Exception $exception) { - if ($this->force) { - $this->writer->warning($exception); - } else { + if (!$this->force) { throw $exception; } - } - }); - $this->repository->delete($server->id); + // Oh well, just try to delete the database entry we have from the database + // so that the server itself can be deleted. This will leave it dangling on + // the host instance, but we couldn't delete it anyways so not sure how we would + // handle this better anyways. + // + // @see https://github.com/pterodactyl/panel/issues/2085 + $database->delete(); + + Log::warning($exception); + } + } + + $server->delete(); }); } } diff --git a/config/logging.php b/config/logging.php index 7da06f407..bb4470c0f 100644 --- a/config/logging.php +++ b/config/logging.php @@ -1,5 +1,6 @@ 'errorlog', 'level' => 'debug', ], + + 'null' => [ + 'driver' => 'monolog', + 'handler' => NullHandler::class, + ], ], ]; diff --git a/tests/Integration/Services/Servers/ServerDeletionServiceTest.php b/tests/Integration/Services/Servers/ServerDeletionServiceTest.php new file mode 100644 index 000000000..bcc4cce25 --- /dev/null +++ b/tests/Integration/Services/Servers/ServerDeletionServiceTest.php @@ -0,0 +1,166 @@ +set('logging.default', 'null'); + + $this->daemonServerRepository = Mockery::mock(DaemonServerRepository::class); + $this->databaseManagementService = Mockery::mock(DatabaseManagementService::class); + + $this->app->instance(DaemonServerRepository::class, $this->daemonServerRepository); + $this->app->instance(DatabaseManagementService::class, $this->databaseManagementService); + } + + /** + * Reset the log driver. + */ + protected function tearDown(): void + { + config()->set('logging.default', self::$defaultLogger); + self::$defaultLogger = null; + + parent::tearDown(); + } + + /** + * Test that a server is not deleted if the force option is not set and an error + * is returned by wings. + */ + public function testRegularDeleteFailsIfWingsReturnsError() + { + $server = $this->createServerModel(); + + $this->expectException(DaemonConnectionException::class); + + $this->daemonServerRepository->expects('setServer->delete')->withNoArgs()->andThrows( + new DaemonConnectionException(new BadResponseException('Bad request', new Request('GET', '/test'))) + ); + + $this->getService()->handle($server); + + $this->assertDatabaseHas('servers', ['id' => $server->id]); + } + + /** + * Test that a 404 from Wings while deleting a server does not cause the deletion to fail. + */ + public function testRegularDeleteIgnores404FromWings() + { + $server = $this->createServerModel(); + + $this->daemonServerRepository->expects('setServer->delete')->withNoArgs()->andThrows( + new DaemonConnectionException(new BadResponseException('Bad request', new Request('GET', '/test'), new Response(404))) + ); + + $this->getService()->handle($server); + + $this->assertDatabaseMissing('servers', ['id' => $server->id]); + } + + /** + * Test that an error from Wings does not cause the deletion to fail if the server is being + * force deleted. + */ + public function testForceDeleteIgnoresExceptionFromWings() + { + $server = $this->createServerModel(); + + $this->daemonServerRepository->expects('setServer->delete')->withNoArgs()->andThrows( + new DaemonConnectionException(new BadResponseException('Bad request', new Request('GET', '/test'), new Response(500))) + ); + + $this->getService()->withForce(true)->handle($server); + + $this->assertDatabaseMissing('servers', ['id' => $server->id]); + } + + /** + * Test that a non-force-delete call does not delete the server if one of the databases + * cannot be deleted from the host. + */ + public function testExceptionWhileDeletingStopsProcess() + { + $server = $this->createServerModel(); + $host = factory(DatabaseHost::class)->create(); + + /** @var \Pterodactyl\Models\Database $db */ + $db = factory(Database::class)->create(['database_host_id' => $host->id, 'server_id' => $server->id]); + + $server->refresh(); + + $this->daemonServerRepository->expects('setServer->delete')->withNoArgs()->andReturnUndefined(); + $this->databaseManagementService->expects('delete')->with(Mockery::on(function ($value) use ($db) { + return $value instanceof Database && $value->id === $db->id; + }))->andThrows(new Exception); + + $this->expectException(Exception::class); + $this->getService()->handle($server); + + $this->assertDatabaseHas('servers', ['id' => $server->id]); + $this->assertDatabaseHas('databases', ['id' => $db->id]); + } + + /** + * Test that a server is deleted even if the server databases cannot be deleted from the host. + */ + public function testExceptionWhileDeletingDatabasesDoesNotAbortIfForceDeleted() + { + $server = $this->createServerModel(); + $host = factory(DatabaseHost::class)->create(); + + /** @var \Pterodactyl\Models\Database $db */ + $db = factory(Database::class)->create(['database_host_id' => $host->id, 'server_id' => $server->id]); + + $server->refresh(); + + $this->daemonServerRepository->expects('setServer->delete')->withNoArgs()->andReturnUndefined(); + $this->databaseManagementService->expects('delete')->with(Mockery::on(function ($value) use ($db) { + return $value instanceof Database && $value->id === $db->id; + }))->andThrows(new Exception); + + $this->getService()->withForce(true)->handle($server); + + $this->assertDatabaseMissing('servers', ['id' => $server->id]); + $this->assertDatabaseMissing('databases', ['id' => $db->id]); + } + + /** + * @return \Pterodactyl\Services\Servers\ServerDeletionService + */ + private function getService() + { + return $this->app->make(ServerDeletionService::class); + } +} diff --git a/tests/Unit/Services/Servers/ServerDeletionServiceTest.php b/tests/Unit/Services/Servers/ServerDeletionServiceTest.php deleted file mode 100644 index d01abab55..000000000 --- a/tests/Unit/Services/Servers/ServerDeletionServiceTest.php +++ /dev/null @@ -1,156 +0,0 @@ -connection = m::mock(ConnectionInterface::class); - $this->daemonServerRepository = m::mock(DaemonServerRepositoryInterface::class); - $this->databaseRepository = m::mock(DatabaseRepositoryInterface::class); - $this->databaseManagementService = m::mock(DatabaseManagementService::class); - $this->repository = m::mock(ServerRepositoryInterface::class); - $this->writer = m::mock(Writer::class); - } - - /** - * Test that a server can be force deleted by setting it in a function call. - */ - public function testForceParameterCanBeSet() - { - $response = $this->getService()->withForce(true); - - $this->assertInstanceOf(ServerDeletionService::class, $response); - } - - /** - * Test that a server can be deleted when force is not set. - */ - public function testServerCanBeDeletedWithoutForce() - { - $model = factory(Server::class)->make(); - - $this->daemonServerRepository->shouldReceive('setServer')->once()->with($model)->andReturnSelf(); - $this->daemonServerRepository->shouldReceive('delete')->once()->withNoArgs()->andReturn(new Response); - - $this->connection->shouldReceive('beginTransaction')->once()->withNoArgs()->andReturnNull(); - $this->databaseRepository->shouldReceive('setColumns')->once()->with('id')->andReturnSelf(); - $this->databaseRepository->shouldReceive('findWhere')->once()->with([ - ['server_id', '=', $model->id], - ])->andReturn(collect([(object) ['id' => 50]])); - - $this->databaseManagementService->shouldReceive('delete')->once()->with(50)->andReturnNull(); - $this->repository->shouldReceive('delete')->once()->with($model->id)->andReturn(1); - $this->connection->shouldReceive('commit')->once()->withNoArgs()->andReturnNull(); - - $this->getService()->handle($model); - } - - /** - * Test that a server is deleted when force is set. - */ - public function testServerShouldBeDeletedEvenWhenFailureOccursIfForceIsSet() - { - $this->configureExceptionMock(); - $model = factory(Server::class)->make(); - - $this->daemonServerRepository->shouldReceive('setServer')->once()->with($model)->andReturnSelf(); - $this->daemonServerRepository->shouldReceive('delete')->once()->withNoArgs()->andThrow($this->getExceptionMock()); - - $this->writer->shouldReceive('warning')->with($this->exception)->once()->andReturnNull(); - $this->connection->shouldReceive('beginTransaction')->withNoArgs()->once()->andReturnNull(); - $this->databaseRepository->shouldReceive('setColumns')->with('id')->once()->andReturnSelf(); - $this->databaseRepository->shouldReceive('findWhere')->with([ - ['server_id', '=', $model->id], - ])->once()->andReturn(collect([(object) ['id' => 50]])); - - $this->databaseManagementService->shouldReceive('delete')->with(50)->once()->andReturnNull(); - $this->repository->shouldReceive('delete')->with($model->id)->once()->andReturn(1); - $this->connection->shouldReceive('commit')->withNoArgs()->once()->andReturnNull(); - - $this->getService()->withForce()->handle($model); - } - - /** - * Test that an exception is thrown if a server cannot be deleted from the node and force is not set. - * - * @expectedException \Pterodactyl\Exceptions\Http\Connection\DaemonConnectionException - */ - public function testExceptionShouldBeThrownIfDaemonReturnsAnErrorAndForceIsNotSet() - { - $this->configureExceptionMock(); - $model = factory(Server::class)->make(); - - $this->daemonServerRepository->shouldReceive('setServer->delete')->once()->andThrow($this->getExceptionMock()); - - $this->getService()->handle($model); - } - - /** - * Return an instance of the class with mocked dependencies. - * - * @return \Pterodactyl\Services\Servers\ServerDeletionService - */ - private function getService(): ServerDeletionService - { - return new ServerDeletionService( - $this->connection, - $this->daemonServerRepository, - $this->databaseRepository, - $this->databaseManagementService, - $this->repository, - $this->writer - ); - } -}