Fix deleting a backup that is locked and failed; closes #3404

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(),

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();
         }
 

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(),
         ];
     }
 }

tests/Integration/Services/Backups/DeleteBackupServiceTest.php

@@ -0,0 +1,123 @@
+<?php
+
+namespace Pterodactyl\Tests\Integration\Services\Backups;
+
+use Mockery;
+use GuzzleHttp\Psr7\Request;
+use GuzzleHttp\Psr7\Response;
+use Pterodactyl\Models\Backup;
+use GuzzleHttp\Exception\ClientException;
+use Pterodactyl\Extensions\Backups\BackupManager;
+use Pterodactyl\Services\Backups\DeleteBackupService;
+use Pterodactyl\Tests\Integration\IntegrationTestCase;
+use Pterodactyl\Repositories\Wings\DaemonBackupRepository;
+use Pterodactyl\Exceptions\Service\Backup\BackupLockedException;
+use Pterodactyl\Exceptions\Http\Connection\DaemonConnectionException;
+
+class DeleteBackupServiceTest extends IntegrationTestCase
+{
+    private $repository;
+
+    public function setUp(): void
+    {
+        parent::setUp();
+
+        $this->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);
+    }
+}