From d049839ffc69a939d3aeec895040d6bfbfaba52d Mon Sep 17 00:00:00 2001 From: Dane Everitt Date: Sun, 13 Jun 2021 10:26:47 -0700 Subject: [PATCH] Fix deleting a backup that is locked and failed; closes #3404 --- .../Remote/Backups/BackupStatusController.php | 13 +- app/Services/Backups/DeleteBackupService.php | 11 +- database/Factories/BackupFactory.php | 5 +- .../Backups/DeleteBackupServiceTest.php | 123 ++++++++++++++++++ 4 files changed, 141 insertions(+), 11 deletions(-) create mode 100644 tests/Integration/Services/Backups/DeleteBackupServiceTest.php diff --git a/app/Http/Controllers/Api/Remote/Backups/BackupStatusController.php b/app/Http/Controllers/Api/Remote/Backups/BackupStatusController.php index 3ec271186..943893eba 100644 --- a/app/Http/Controllers/Api/Remote/Backups/BackupStatusController.php +++ b/app/Http/Controllers/Api/Remote/Backups/BackupStatusController.php @@ -12,17 +12,11 @@ use League\Flysystem\AwsS3v3\AwsS3Adapter; use Pterodactyl\Exceptions\DisplayException; use Pterodactyl\Http\Controllers\Controller; use Pterodactyl\Extensions\Backups\BackupManager; -use Pterodactyl\Repositories\Eloquent\BackupRepository; use Symfony\Component\HttpKernel\Exception\BadRequestHttpException; use Pterodactyl\Http\Requests\Api\Remote\ReportBackupCompleteRequest; class BackupStatusController extends Controller { - /** - * @var \Pterodactyl\Repositories\Eloquent\BackupRepository - */ - private $repository; - /** * @var \Pterodactyl\Extensions\Backups\BackupManager */ @@ -31,9 +25,8 @@ class BackupStatusController extends Controller /** * BackupStatusController constructor. */ - public function __construct(BackupRepository $repository, BackupManager $backupManager) + public function __construct(BackupManager $backupManager) { - $this->repository = $repository; $this->backupManager = $backupManager; } @@ -64,6 +57,10 @@ class BackupStatusController extends Controller $successful = $request->input('successful') ? true : false; $model->fill([ 'is_successful' => $successful, + // Change the lock state to unlocked if this was a failed backup so that it can be + // deleted easily. Also does not make sense to have a locked backup on the system + // that is failed. + 'is_locked' => $successful ? $model->is_locked : false, 'checksum' => $successful ? ($request->input('checksum_type') . ':' . $request->input('checksum')) : null, 'bytes' => $successful ? $request->input('size') : 0, 'completed_at' => CarbonImmutable::now(), diff --git a/app/Services/Backups/DeleteBackupService.php b/app/Services/Backups/DeleteBackupService.php index 772392e21..80d6374b1 100644 --- a/app/Services/Backups/DeleteBackupService.php +++ b/app/Services/Backups/DeleteBackupService.php @@ -50,13 +50,20 @@ class DeleteBackupService } /** - * Deletes a backup from the system. + * Deletes a backup from the system. If the backup is stored in S3 a request + * will be made to delete that backup from the disk as well. * * @throws \Throwable */ public function handle(Backup $backup) { - if ($backup->is_locked) { + // If the backup is marked as failed it can still be deleted, even if locked + // since the UI doesn't allow you to unlock a failed backup in the first place. + // + // I also don't really see any reason you'd have a locked, failed backup to keep + // around. The logic that updates the backup to the failed state will also remove + // the lock, so this condition should really never happen. + if ($backup->is_locked && ($backup->completed_at && $backup->is_successful)) { throw new BackupLockedException(); } diff --git a/database/Factories/BackupFactory.php b/database/Factories/BackupFactory.php index de9a1d923..4333ee34e 100644 --- a/database/Factories/BackupFactory.php +++ b/database/Factories/BackupFactory.php @@ -3,6 +3,7 @@ namespace Database\Factories; use Ramsey\Uuid\Uuid; +use Carbon\CarbonImmutable; use Pterodactyl\Models\Backup; use Illuminate\Database\Eloquent\Factories\Factory; @@ -22,9 +23,11 @@ class BackupFactory extends Factory { return [ 'uuid' => Uuid::uuid4()->toString(), - 'is_successful' => true, 'name' => $this->faker->sentence, 'disk' => Backup::ADAPTER_WINGS, + 'is_successful' => true, + 'created_at' => CarbonImmutable::now(), + 'completed_at' => CarbonImmutable::now(), ]; } } diff --git a/tests/Integration/Services/Backups/DeleteBackupServiceTest.php b/tests/Integration/Services/Backups/DeleteBackupServiceTest.php new file mode 100644 index 000000000..34d4028bd --- /dev/null +++ b/tests/Integration/Services/Backups/DeleteBackupServiceTest.php @@ -0,0 +1,123 @@ +repository = Mockery::mock(DaemonBackupRepository::class); + + $this->app->instance(DaemonBackupRepository::class, $this->repository); + } + + public function testLockedBackupCannotBeDeleted() + { + $server = $this->createServerModel(); + $backup = Backup::factory()->create([ + 'server_id' => $server->id, + 'is_locked' => true, + ]); + + $this->expectException(BackupLockedException::class); + + $this->app->make(DeleteBackupService::class)->handle($backup); + } + + public function testFailedBackupThatIsLockedCanBeDeleted() + { + $server = $this->createServerModel(); + $backup = Backup::factory()->create([ + 'server_id' => $server->id, + 'is_locked' => true, + 'is_successful' => false, + ]); + + $this->repository->expects('setServer->delete')->with($backup)->andReturn( + new Response() + ); + + $this->app->make(DeleteBackupService::class)->handle($backup); + + $backup->refresh(); + + $this->assertNotNull($backup->deleted_at); + } + + public function testExceptionThrownDueToMissingBackupIsIgnored() + { + $server = $this->createServerModel(); + $backup = Backup::factory()->create(['server_id' => $server->id]); + + $this->repository->expects('setServer->delete')->with($backup)->andThrow( + new DaemonConnectionException( + new ClientException('', new Request('DELETE', '/'), new Response(404)) + ) + ); + + $this->app->make(DeleteBackupService::class)->handle($backup); + + $backup->refresh(); + + $this->assertNotNull($backup->deleted_at); + } + + public function testExceptionIsThrownIfNot404() + { + $server = $this->createServerModel(); + $backup = Backup::factory()->create(['server_id' => $server->id]); + + $this->repository->expects('setServer->delete')->with($backup)->andThrow( + new DaemonConnectionException( + new ClientException('', new Request('DELETE', '/'), new Response(500)) + ) + ); + + $this->expectException(DaemonConnectionException::class); + + $this->app->make(DeleteBackupService::class)->handle($backup); + + $backup->refresh(); + + $this->assertNull($backup->deleted_at); + } + + public function testS3ObjectCanBeDeleted() + { + $server = $this->createServerModel(); + $backup = Backup::factory()->create([ + 'disk' => Backup::ADAPTER_AWS_S3, + 'server_id' => $server->id, + ]); + + $manager = $this->mock(BackupManager::class); + $manager->expects('getBucket')->andReturns('foobar'); + $manager->expects('adapter')->with(Backup::ADAPTER_AWS_S3)->andReturnSelf(); + $manager->expects('getClient->deleteObject')->with([ + 'Bucket' => 'foobar', + 'Key' => sprintf('%s/%s.tar.gz', $server->uuid, $backup->uuid), + ]); + + $this->app->make(DeleteBackupService::class)->handle($backup); + + $backup->refresh(); + + $this->assertNotNull($backup->deleted_at); + } +}