fix(private-key): implement transaction handling and error verification for private key storage operations

app/Models/PrivateKey.php

@@ -4,6 +4,7 @@ namespace App\Models;
 
 use App\Traits\HasSafeStringAttribute;
 use DanHarrin\LivewireRateLimiting\WithRateLimiting;
+use Illuminate\Support\Facades\DB;
 use Illuminate\Support\Facades\Storage;
 use Illuminate\Validation\ValidationException;
 use OpenApi\Attributes as OA;
@@ -99,11 +100,18 @@ class PrivateKey extends BaseModel
 
     public static function createAndStore(array $data)
     {
-        $privateKey = new self($data);
+        return DB::transaction(function () use ($data) {
-        $privateKey->save();
+            $privateKey = new self($data);
-        $privateKey->storeInFileSystem();
+            $privateKey->save();
 
-        return $privateKey;
+            try {
+                $privateKey->storeInFileSystem();
+            } catch (\Exception $e) {
+                throw new \Exception('Failed to store SSH key: '.$e->getMessage());
+            }
+
+            return $privateKey;
+        });
     }
 
     public static function generateNewKeyPair($type = 'rsa')
@@ -150,16 +158,66 @@ class PrivateKey extends BaseModel
 
     public function storeInFileSystem()
     {
+        ray('storing private key in filesystem', $this->uuid);
         $filename = "ssh_key@{$this->uuid}";
-        Storage::disk('ssh-keys')->put($filename, $this->private_key);
+        $disk = Storage::disk('ssh-keys');
 
-        return "/var/www/html/storage/app/ssh/keys/{$filename}";
+        // Ensure the storage directory exists and is writable
+        $this->ensureStorageDirectoryExists();
+
+        // Attempt to store the private key
+        $success = $disk->put($filename, $this->private_key);
+
+        if (! $success) {
+            throw new \Exception("Failed to write SSH key to filesystem. Check disk space and permissions for: {$this->getKeyLocation()}");
+        }
+
+        // Verify the file was actually created and has content
+        if (! $disk->exists($filename)) {
+            throw new \Exception("SSH key file was not created: {$this->getKeyLocation()}");
+        }
+
+        $storedContent = $disk->get($filename);
+        if (empty($storedContent) || $storedContent !== $this->private_key) {
+            $disk->delete($filename); // Clean up the bad file
+            throw new \Exception("SSH key file content verification failed: {$this->getKeyLocation()}");
+        }
+
+        return $this->getKeyLocation();
     }
 
     public static function deleteFromStorage(self $privateKey)
     {
         $filename = "ssh_key@{$privateKey->uuid}";
-        Storage::disk('ssh-keys')->delete($filename);
+        $disk = Storage::disk('ssh-keys');
+
+        if ($disk->exists($filename)) {
+            $disk->delete($filename);
+        }
+    }
+
+    protected function ensureStorageDirectoryExists()
+    {
+        $disk = Storage::disk('ssh-keys');
+        $directoryPath = '';
+
+        if (! $disk->exists($directoryPath)) {
+            $success = $disk->makeDirectory($directoryPath);
+            if (! $success) {
+                throw new \Exception('Failed to create SSH keys storage directory');
+            }
+        }
+
+        // Check if directory is writable by attempting a test file
+        $testFilename = '.test_write_'.uniqid();
+        $testSuccess = $disk->put($testFilename, 'test');
+
+        if (! $testSuccess) {
+            throw new \Exception('SSH keys storage directory is not writable');
+        }
+
+        // Clean up test file
+        $disk->delete($testFilename);
     }
 
     public function getKeyLocation()
@@ -169,10 +227,17 @@ class PrivateKey extends BaseModel
 
     public function updatePrivateKey(array $data)
     {
-        $this->update($data);
+        return DB::transaction(function () use ($data) {
-        $this->storeInFileSystem();
+            $this->update($data);
 
-        return $this;
+            try {
+                $this->storeInFileSystem();
+            } catch (\Exception $e) {
+                throw new \Exception('Failed to update SSH key: '.$e->getMessage());
+            }
+
+            return $this;
+        });
     }
 
     public function servers()

tests/Unit/PrivateKeyStorageTest.php

@@ -0,0 +1,316 @@
+<?php
+
+use App\Models\PrivateKey;
+use Illuminate\Foundation\Testing\RefreshDatabase;
+use Illuminate\Support\Facades\Storage;
+use Tests\TestCase;
+
+class PrivateKeyStorageTest extends TestCase
+{
+    use RefreshDatabase;
+
+    protected function setUp(): void
+    {
+        parent::setUp();
+
+        // Set up a test team for the tests
+        $this->actingAs(\App\Models\User::factory()->create());
+    }
+
+    protected function getValidPrivateKey(): string
+    {
+        return '-----BEGIN OPENSSH PRIVATE KEY-----
+b3BlbnNzaC1rZXktdjEAAAAABG5vbmUAAAAEbm9uZQAAAAAAAAABAAAAMwAAAAtzc2gtZW
+QyNTUxOQAAACBbhpqHhqv6aI67Mj9abM3DVbmcfYhZAhC7ca4d9UCevAAAAJi/QySHv0Mk
+hwAAAAtzc2gtZWQyNTUxOQAAACBbhpqHhqv6aI67Mj9abM3DVbmcfYhZAhC7ca4d9UCevA
+AAAECBQw4jg1WRT2IGHMncCiZhURCts2s24HoDS0thHnnRKVuGmoeGq/pojrsyP1pszcNV
+uZx9iFkCELtxrh31QJ68AAAAEXNhaWxANzZmZjY2ZDJlMmRkAQIDBA==
+-----END OPENSSH PRIVATE KEY-----';
+    }
+
+    /** @test */
+    public function it_successfully_stores_private_key_in_filesystem()
+    {
+        Storage::fake('ssh-keys');
+
+        $privateKey = PrivateKey::createAndStore([
+            'name' => 'Test Key',
+            'description' => 'Test Description',
+            'private_key' => $this->getValidPrivateKey(),
+            'team_id' => currentTeam()->id,
+        ]);
+
+        $this->assertDatabaseHas('private_keys', [
+            'id' => $privateKey->id,
+            'name' => 'Test Key',
+        ]);
+
+        $filename = "ssh_key@{$privateKey->uuid}";
+        Storage::disk('ssh-keys')->assertExists($filename);
+
+        $storedContent = Storage::disk('ssh-keys')->get($filename);
+        $this->assertEquals($privateKey->private_key, $storedContent);
+    }
+
+    /** @test */
+    public function it_throws_exception_when_storage_fails()
+    {
+        Storage::fake('ssh-keys');
+
+        // Mock Storage::put to return false (simulating storage failure)
+        Storage::shouldReceive('disk')
+            ->with('ssh-keys')
+            ->andReturn(
+                \Mockery::mock()
+                    ->shouldReceive('exists')
+                    ->andReturn(true)
+                    ->shouldReceive('put')
+                    ->with(\Mockery::any(), 'test')
+                    ->andReturn(true)
+                    ->shouldReceive('delete')
+                    ->with(\Mockery::any())
+                    ->andReturn(true)
+                    ->shouldReceive('put')
+                    ->with(\Mockery::pattern('/ssh_key@/'), \Mockery::any())
+                    ->andReturn(false) // Simulate storage failure
+                    ->getMock()
+            );
+
+        $this->expectException(\Exception::class);
+        $this->expectExceptionMessage('Failed to write SSH key to filesystem');
+
+        PrivateKey::createAndStore([
+            'name' => 'Test Key',
+            'description' => 'Test Description',
+            'private_key' => $this->getValidPrivateKey(),
+            'team_id' => currentTeam()->id,
+        ]);
+
+        // Assert that no database record was created due to transaction rollback
+        $this->assertDatabaseMissing('private_keys', [
+            'name' => 'Test Key',
+        ]);
+    }
+
+    /** @test */
+    public function it_throws_exception_when_storage_directory_is_not_writable()
+    {
+        Storage::fake('ssh-keys');
+
+        // Mock Storage disk to simulate directory not writable
+        Storage::shouldReceive('disk')
+            ->with('ssh-keys')
+            ->andReturn(
+                \Mockery::mock()
+                    ->shouldReceive('exists')
+                    ->with('')
+                    ->andReturn(true)
+                    ->shouldReceive('put')
+                    ->with(\Mockery::pattern('/\.test_write_/'), 'test')
+                    ->andReturn(false) // Simulate directory not writable
+                    ->getMock()
+            );
+
+        $this->expectException(\Exception::class);
+        $this->expectExceptionMessage('SSH keys storage directory is not writable');
+
+        PrivateKey::createAndStore([
+            'name' => 'Test Key',
+            'description' => 'Test Description',
+            'private_key' => $this->getValidPrivateKey(),
+            'team_id' => currentTeam()->id,
+        ]);
+    }
+
+    /** @test */
+    public function it_creates_storage_directory_if_not_exists()
+    {
+        Storage::fake('ssh-keys');
+
+        // Mock Storage disk to simulate directory not existing, then being created
+        Storage::shouldReceive('disk')
+            ->with('ssh-keys')
+            ->andReturn(
+                \Mockery::mock()
+                    ->shouldReceive('exists')
+                    ->with('')
+                    ->andReturn(false) // Directory doesn't exist
+                    ->shouldReceive('makeDirectory')
+                    ->with('')
+                    ->andReturn(true) // Successfully create directory
+                    ->shouldReceive('put')
+                    ->with(\Mockery::pattern('/\.test_write_/'), 'test')
+                    ->andReturn(true) // Directory is writable after creation
+                    ->shouldReceive('delete')
+                    ->with(\Mockery::pattern('/\.test_write_/'))
+                    ->andReturn(true)
+                    ->shouldReceive('put')
+                    ->with(\Mockery::pattern('/ssh_key@/'), \Mockery::any())
+                    ->andReturn(true)
+                    ->shouldReceive('exists')
+                    ->with(\Mockery::pattern('/ssh_key@/'))
+                    ->andReturn(true)
+                    ->shouldReceive('get')
+                    ->with(\Mockery::pattern('/ssh_key@/'))
+                    ->andReturn($this->getValidPrivateKey())
+                    ->getMock()
+            );
+
+        $privateKey = PrivateKey::createAndStore([
+            'name' => 'Test Key',
+            'description' => 'Test Description',
+            'private_key' => $this->getValidPrivateKey(),
+            'team_id' => currentTeam()->id,
+        ]);
+
+        $this->assertDatabaseHas('private_keys', [
+            'id' => $privateKey->id,
+            'name' => 'Test Key',
+        ]);
+    }
+
+    /** @test */
+    public function it_throws_exception_when_file_content_verification_fails()
+    {
+        Storage::fake('ssh-keys');
+
+        // Mock Storage disk to simulate file being created but with wrong content
+        Storage::shouldReceive('disk')
+            ->with('ssh-keys')
+            ->andReturn(
+                \Mockery::mock()
+                    ->shouldReceive('exists')
+                    ->with('')
+                    ->andReturn(true)
+                    ->shouldReceive('put')
+                    ->with(\Mockery::pattern('/\.test_write_/'), 'test')
+                    ->andReturn(true)
+                    ->shouldReceive('delete')
+                    ->with(\Mockery::pattern('/\.test_write_/'))
+                    ->andReturn(true)
+                    ->shouldReceive('put')
+                    ->with(\Mockery::pattern('/ssh_key@/'), \Mockery::any())
+                    ->andReturn(true) // File created successfully
+                    ->shouldReceive('exists')
+                    ->with(\Mockery::pattern('/ssh_key@/'))
+                    ->andReturn(true) // File exists
+                    ->shouldReceive('get')
+                    ->with(\Mockery::pattern('/ssh_key@/'))
+                    ->andReturn('corrupted content') // But content is wrong
+                    ->shouldReceive('delete')
+                    ->with(\Mockery::pattern('/ssh_key@/'))
+                    ->andReturn(true) // Clean up bad file
+                    ->getMock()
+            );
+
+        $this->expectException(\Exception::class);
+        $this->expectExceptionMessage('SSH key file content verification failed');
+
+        PrivateKey::createAndStore([
+            'name' => 'Test Key',
+            'description' => 'Test Description',
+            'private_key' => $this->getValidPrivateKey(),
+            'team_id' => currentTeam()->id,
+        ]);
+
+        // Assert that no database record was created due to transaction rollback
+        $this->assertDatabaseMissing('private_keys', [
+            'name' => 'Test Key',
+        ]);
+    }
+
+    /** @test */
+    public function it_successfully_deletes_private_key_from_filesystem()
+    {
+        Storage::fake('ssh-keys');
+
+        $privateKey = PrivateKey::createAndStore([
+            'name' => 'Test Key',
+            'description' => 'Test Description',
+            'private_key' => $this->getValidPrivateKey(),
+            'team_id' => currentTeam()->id,
+        ]);
+
+        $filename = "ssh_key@{$privateKey->uuid}";
+        Storage::disk('ssh-keys')->assertExists($filename);
+
+        $privateKey->delete();
+
+        Storage::disk('ssh-keys')->assertMissing($filename);
+    }
+
+    /** @test */
+    public function it_handles_database_transaction_rollback_on_storage_failure()
+    {
+        Storage::fake('ssh-keys');
+
+        // Count initial private keys
+        $initialCount = PrivateKey::count();
+
+        // Mock storage failure after database save
+        Storage::shouldReceive('disk')
+            ->with('ssh-keys')
+            ->andReturn(
+                \Mockery::mock()
+                    ->shouldReceive('exists')
+                    ->with('')
+                    ->andReturn(true)
+                    ->shouldReceive('put')
+                    ->with(\Mockery::pattern('/\.test_write_/'), 'test')
+                    ->andReturn(true)
+                    ->shouldReceive('delete')
+                    ->with(\Mockery::pattern('/\.test_write_/'))
+                    ->andReturn(true)
+                    ->shouldReceive('put')
+                    ->with(\Mockery::pattern('/ssh_key@/'), \Mockery::any())
+                    ->andReturn(false) // Storage fails
+                    ->getMock()
+            );
+
+        try {
+            PrivateKey::createAndStore([
+                'name' => 'Test Key',
+                'description' => 'Test Description',
+                'private_key' => $this->getValidPrivateKey(),
+                'team_id' => currentTeam()->id,
+            ]);
+        } catch (\Exception $e) {
+            // Expected exception
+        }
+
+        // Assert that database was rolled back
+        $this->assertEquals($initialCount, PrivateKey::count());
+        $this->assertDatabaseMissing('private_keys', [
+            'name' => 'Test Key',
+        ]);
+    }
+
+    /** @test */
+    public function it_successfully_updates_private_key_with_transaction()
+    {
+        Storage::fake('ssh-keys');
+
+        $privateKey = PrivateKey::createAndStore([
+            'name' => 'Test Key',
+            'description' => 'Test Description',
+            'private_key' => $this->getValidPrivateKey(),
+            'team_id' => currentTeam()->id,
+        ]);
+
+        $newPrivateKey = str_replace('Test', 'Updated', $this->getValidPrivateKey());
+
+        $privateKey->updatePrivateKey([
+            'name' => 'Updated Key',
+            'private_key' => $newPrivateKey,
+        ]);
+
+        $this->assertDatabaseHas('private_keys', [
+            'id' => $privateKey->id,
+            'name' => 'Updated Key',
+        ]);
+
+        $filename = "ssh_key@{$privateKey->uuid}";
+        $storedContent = Storage::disk('ssh-keys')->get($filename);
+        $this->assertEquals($newPrivateKey, $storedContent);
+    }
+}