Use presigned URLs for all S3 operations (#1022)

app/Extensions/Filesystem/S3Filesystem.php

@@ -2,11 +2,18 @@
 
 namespace App\Extensions\Filesystem;
 
+use Aws\CommandInterface;
+use Aws\Result;
 use Aws\S3\S3ClientInterface;
+use GuzzleHttp\Client as GuzzleClient;
 use League\Flysystem\AwsS3V3\AwsS3V3Adapter;
+use RuntimeException;
+use SimpleXMLElement;
 
 class S3Filesystem extends AwsS3V3Adapter
 {
+    private ?GuzzleClient $guzzle = null;
+
     /**
      * @param  array<mixed>  $options
      */
@@ -26,6 +33,18 @@ public function __construct(
         );
     }
 
+    private function getGuzzleClient(): GuzzleClient
+    {
+        if ($this->guzzle === null) {
+            $this->guzzle = new GuzzleClient([
+                'timeout' => 30,
+                'connect_timeout' => 10,
+            ]);
+        }
+
+        return $this->guzzle;
+    }
+
     public function getClient(): S3ClientInterface
     {
         return $this->client;
@@ -35,4 +54,78 @@ public function getBucket(): string
     {
         return $this->bucket;
     }
+
+    /**
+     * Execute an S3 command using a presigned URL for maximum compatibility
+     * with S3-compatible providers.
+     *
+     * @return Result<array<string, mixed>>
+     */
+    public function executeS3Command(CommandInterface $command): Result
+    {
+        $presignedRequest = $this->client->createPresignedRequest($command, '+60 minutes');
+
+        $response = $this->getGuzzleClient()->send($presignedRequest);
+
+        $body = (string) $response->getBody();
+        $commandName = $command->getName();
+
+        // S3's CompleteMultipartUpload can return HTTP 200 with an <Error> body
+        if ($body !== '' && str_contains($body, '<Error>')) {
+            throw new RuntimeException("S3 returned an error for $commandName: $body");
+        }
+
+        return new Result($this->parseS3Response($commandName, $body));
+    }
+
+    /**
+     * Parse the XML response body based on the S3 command type.
+     *
+     * @return array<string, mixed>
+     */
+    private function parseS3Response(string $commandName, string $body): array
+    {
+        if ($body === '') {
+            return [];
+        }
+
+        $xml = @simplexml_load_string($body);
+        if ($xml === false) {
+            throw new RuntimeException("Failed to parse S3 XML response for $commandName: $body");
+        }
+
+        return match ($commandName) {
+            'CreateMultipartUpload' => $this->parseCreateMultipartUpload($xml),
+            'ListParts' => $this->parseListParts($xml),
+            'CompleteMultipartUpload' => [],
+            default => [],
+        };
+    }
+
+    /**
+     * @return array{UploadId: string}
+     */
+    private function parseCreateMultipartUpload(SimpleXMLElement $xml): array
+    {
+        return [
+            'UploadId' => (string) $xml->UploadId,
+        ];
+    }
+
+    /**
+     * @return array{Parts: array<int, array{ETag: string, PartNumber: int}>}
+     */
+    private function parseListParts(SimpleXMLElement $xml): array
+    {
+        $parts = [];
+
+        foreach ($xml->Part as $part) {
+            $parts[] = [
+                'ETag' => (string) $part->ETag,
+                'PartNumber' => (int) $part->PartNumber,
+            ];
+        }
+
+        return ['Parts' => $parts];
+    }
 }

app/Http/Controllers/Api/Remote/Backups/BackupRemoteUploadController.php

@@ -91,7 +91,7 @@ public function __invoke(Request $request, string $backup): JsonResponse
         }
 
         // Execute the CreateMultipartUpload request
-        $result = $client->execute($client->getCommand('CreateMultipartUpload', $params));
+        $result = $adapter->executeS3Command($client->getCommand('CreateMultipartUpload', $params));
 
         // Get the UploadId from the CreateMultipartUpload request, this is needed to create
         // the other presigned urls.

app/Http/Controllers/Api/Remote/Backups/BackupStatusController.php

@@ -138,7 +138,7 @@ protected function completeMultipartUpload(Backup $backup, S3Filesystem $adapter
 
         $client = $adapter->getClient();
         if (!$successful) {
-            $client->execute($client->getCommand('AbortMultipartUpload', $params));
+            $adapter->executeS3Command($client->getCommand('AbortMultipartUpload', $params));
 
             return;
         }
@@ -149,7 +149,7 @@ protected function completeMultipartUpload(Backup $backup, S3Filesystem $adapter
         ];
 
         if (is_null($parts)) {
-            $params['MultipartUpload']['Parts'] = $client->execute($client->getCommand('ListParts', $params))['Parts'];
+            $params['MultipartUpload']['Parts'] = $adapter->executeS3Command($client->getCommand('ListParts', $params))['Parts'];
         } else {
             foreach ($parts as $part) {
                 $params['MultipartUpload']['Parts'][] = [
@@ -159,6 +159,6 @@ protected function completeMultipartUpload(Backup $backup, S3Filesystem $adapter
             }
         }
 
-        $client->execute($client->getCommand('CompleteMultipartUpload', $params));
+        $adapter->executeS3Command($client->getCommand('CompleteMultipartUpload', $params));
     }
 }

app/Services/Activity/ActivityLogService.php

@@ -10,7 +10,6 @@
 use Illuminate\Contracts\Auth\Factory as AuthFactory;
 use Illuminate\Database\ConnectionInterface;
 use Illuminate\Database\Eloquent\Model;
-use Illuminate\Support\Arr;
 use Illuminate\Support\Collection;
 use Illuminate\Support\Facades\Request;
 use Throwable;
@@ -71,7 +70,7 @@ public function description(?string $description): self
      */
     public function subject(...$subjects): self
     {
-        foreach (Arr::wrap($subjects) as $subject) {
+        foreach ($subjects as $subject) {
             if (is_null($subject)) {
                 continue;
             }

app/Services/Backups/DeleteBackupService.php

@@ -7,7 +7,6 @@
 use App\Extensions\Filesystem\S3Filesystem;
 use App\Models\Backup;
 use App\Repositories\Daemon\DaemonBackupRepository;
-use Aws\S3\S3Client;
 use Exception;
 use Illuminate\Database\ConnectionInterface;
 use Illuminate\Http\Response;
@@ -72,14 +71,12 @@ protected function deleteFromS3(Backup $backup): void
 
             /** @var S3Filesystem $adapter */
             $adapter = $this->manager->adapter(Backup::ADAPTER_AWS_S3);
-
-            /** @var S3Client $client */
             $client = $adapter->getClient();
 
-            $client->deleteObject([
+            $adapter->executeS3Command($client->getCommand('DeleteObject', [
                 'Bucket' => $adapter->getBucket(),
                 'Key' => sprintf('%s/%s.tar.gz', $backup->server->uuid, $backup->uuid),
-            ]);
+            ]));
         });
     }
 }

tests/Integration/Services/Backups/DeleteBackupServiceTest.php

@@ -9,8 +9,10 @@
 use App\Repositories\Daemon\DaemonBackupRepository;
 use App\Services\Backups\DeleteBackupService;
 use App\Tests\Integration\IntegrationTestCase;
+use Aws\CommandInterface;
 use GuzzleHttp\Psr7\Response;
 use Illuminate\Http\Client\ConnectionException;
+use Mockery;
 
 class DeleteBackupServiceTest extends IntegrationTestCase
 {
@@ -92,10 +94,12 @@ public function test_s3_object_can_be_deleted(): void
         $manager->expects('adapter')->with(Backup::ADAPTER_AWS_S3)->andReturn($adapter);
 
         $adapter->expects('getBucket')->andReturn('foobar');
-        $adapter->expects('getClient->deleteObject')->with([
+        $mockCommand = Mockery::mock(CommandInterface::class);
+        $adapter->expects('getClient->getCommand')->with('DeleteObject', [
             'Bucket' => 'foobar',
             'Key' => sprintf('%s/%s.tar.gz', $server->uuid, $backup->uuid),
-        ]);
+        ])->andReturn($mockCommand);
+        $adapter->expects('executeS3Command')->with($mockCommand);
 
         $this->app->make(DeleteBackupService::class)->handle($backup);