Disconnect SFTP/Websocket when a user is removed as a subuser (#5472)

DaneEveritt · web-flow · commit 2bd9d8baddb0 · 2025-12-26T17:51:25.000-08:00
diff --git a/app/Http/Controllers/Api/Client/Servers/SubuserController.php b/app/Http/Controllers/Api/Client/Servers/SubuserController.php
@@ -10,8 +10,8 @@
 use Illuminate\Support\Facades\Log;
 use Pterodactyl\Repositories\Eloquent\SubuserRepository;
 use Pterodactyl\Services\Subusers\SubuserCreationService;
-use Pterodactyl\Repositories\Wings\DaemonServerRepository;
 use Pterodactyl\Transformers\Api\Client\SubuserTransformer;
+use Pterodactyl\Repositories\Wings\DaemonRevocationRepository;
 use Pterodactyl\Http\Controllers\Api\Client\ClientApiController;
 use Pterodactyl\Exceptions\Http\Connection\DaemonConnectionException;
 use Pterodactyl\Http\Requests\Api\Client\Servers\Subusers\GetSubuserRequest;
@@ -27,7 +27,7 @@ class SubuserController extends ClientApiController
     public function __construct(
         private SubuserRepository $repository,
         private SubuserCreationService $creationService,
-        private DaemonServerRepository $serverRepository,
+        private DaemonRevocationRepository $revocationRepository,
     ) {
         parent::__construct();
     }
@@ -115,7 +115,10 @@ public function update(UpdateSubuserRequest $request, Server $server): array
                 ]);
 
                 try {
-                    $this->serverRepository->setServer($server)->revokeUserJTI($subuser->user_id);
+                    $this->revocationRepository->setNode($server->node)->deauthorize(
+                        $subuser->user->uuid,
+                        [$server->uuid],
+                    );
                 } catch (DaemonConnectionException $exception) {
                     // Don't block this request if we can't connect to the Wings instance. Chances are it is
                     // offline and the token will be invalid once Wings boots back.
@@ -150,7 +153,10 @@ public function delete(DeleteSubuserRequest $request, Server $server): JsonRespo
             $subuser->delete();
 
             try {
-                $this->serverRepository->setServer($server)->revokeUserJTI($subuser->user_id);
+                $this->revocationRepository->setNode($server->node)->deauthorize(
+                    $subuser->user->uuid,
+                    [$server->uuid],
+                );
             } catch (DaemonConnectionException $exception) {
                 // Don't block this request if we can't connect to the Wings instance.
                 Log::warning($exception, ['user_id' => $subuser->user_id, 'server_id' => $server->id]);
diff --git a/app/Repositories/Wings/DaemonRepository.php b/app/Repositories/Wings/DaemonRepository.php
@@ -8,10 +8,6 @@
 use Pterodactyl\Models\Server;
 use Illuminate\Contracts\Foundation\Application;
 
-/**
- * @method \Pterodactyl\Repositories\Wings\DaemonRepository setNode(\Pterodactyl\Models\Node $node)
- * @method \Pterodactyl\Repositories\Wings\DaemonRepository setServer(\Pterodactyl\Models\Server $server)
- */
 abstract class DaemonRepository
 {
     protected ?Server $server;
diff --git a/app/Repositories/Wings/DaemonRevocationRepository.php b/app/Repositories/Wings/DaemonRevocationRepository.php
@@ -0,0 +1,27 @@
+<?php
+
+namespace Pterodactyl\Repositories\Wings;
+
+use GuzzleHttp\Exception\TransferException;
+use Pterodactyl\Exceptions\Http\Connection\DaemonConnectionException;
+
+class DaemonRevocationRepository extends DaemonRepository
+{
+    /**
+     * Deauthorizes a user (disconnects websockets and SFTP) on the Wings instance for
+     * the provided servers. If no servers are provided, the user is deauthorized on all
+     * servers on the instance.
+     *
+     * @param string[] $servers
+     */
+    public function deauthorize(string $user, array $servers = []): void
+    {
+        try {
+            $this->getHttpClient()->post('/api/deauthorize-user', [
+                'json' => ['user' => $user, 'servers' => $servers],
+            ]);
+        } catch (TransferException $exception) {
+            throw new DaemonConnectionException($exception);
+        }
+    }
+}
diff --git a/app/Repositories/Wings/DaemonServerRepository.php b/app/Repositories/Wings/DaemonServerRepository.php
@@ -131,6 +131,9 @@ public function requestArchive(): void
      * make it easier to revoke tokens on the fly. This ensures that the JTI key is formatted
      * correctly and avoids any costly mistakes in the codebase.
      *
+     * @deprecated
+     * @see \Pterodactyl\Repositories\Wings\DaemonRevocationRepository::deauthorize()
+     *
      * @throws DaemonConnectionException
      */
     public function revokeUserJTI(int $id): void
diff --git a/app/Services/Servers/DetailsModificationService.php b/app/Services/Servers/DetailsModificationService.php
@@ -7,6 +7,7 @@
 use Illuminate\Database\ConnectionInterface;
 use Pterodactyl\Traits\Services\ReturnsUpdatedModels;
 use Pterodactyl\Repositories\Wings\DaemonServerRepository;
+use Pterodactyl\Repositories\Wings\DaemonRevocationRepository;
 use Pterodactyl\Exceptions\Http\Connection\DaemonConnectionException;
 
 class DetailsModificationService
@@ -16,8 +17,11 @@ class DetailsModificationService
     /**
      * DetailsModificationService constructor.
      */
-    public function __construct(private ConnectionInterface $connection, private DaemonServerRepository $serverRepository)
-    {
+    public function __construct(
+        private ConnectionInterface $connection,
+        private DaemonServerRepository $serverRepository,
+        private DaemonRevocationRepository $revocationRepository,
+    ) {
     }
 
     /**
@@ -28,7 +32,7 @@ public function __construct(private ConnectionInterface $connection, private Dae
     public function handle(Server $server, array $data): Server
     {
         return $this->connection->transaction(function () use ($data, $server) {
-            $owner = $server->owner_id;
+            $original = $server->user;
 
             $server->forceFill([
                 'external_id' => Arr::get($data, 'external_id'),
@@ -40,9 +44,12 @@ public function handle(Server $server, array $data): Server
             // If the owner_id value is changed we need to revoke any tokens that exist for the server
             // on the Wings instance so that the old owner no longer has any permission to access the
             // websockets.
-            if ($server->owner_id !== $owner) {
+            if (! $server->refresh()->user->is($original)) {
                 try {
-                    $this->serverRepository->setServer($server)->revokeUserJTI($owner);
+                    $this->revocationRepository->setNode($server->node)->deauthorize(
+                        $original->uuid,
+                        [$server->uuid],
+                    );
                 } catch (DaemonConnectionException $exception) {
                     // Do nothing. A failure here is not ideal, but it is likely to be caused by Wings
                     // being offline, or in an entirely broken state. Remember, these tokens reset every
diff --git a/tests/Integration/Api/Client/Server/Subuser/DeleteSubuserTest.php b/tests/Integration/Api/Client/Server/Subuser/DeleteSubuserTest.php
@@ -3,10 +3,12 @@
 namespace Pterodactyl\Tests\Integration\Api\Client\Server\Subuser;
 
 use Ramsey\Uuid\Uuid;
+use Mockery\MockInterface;
 use Pterodactyl\Models\User;
 use Pterodactyl\Models\Subuser;
 use Pterodactyl\Models\Permission;
-use Pterodactyl\Repositories\Wings\DaemonServerRepository;
+use PHPUnit\Framework\Attributes\TestWith;
+use Pterodactyl\Repositories\Wings\DaemonRevocationRepository;
 use Pterodactyl\Tests\Integration\Api\Client\ClientApiIntegrationTestCase;
 
 class DeleteSubuserTest extends ClientApiIntegrationTestCase
@@ -22,18 +24,18 @@ class DeleteSubuserTest extends ClientApiIntegrationTestCase
      *
      * @see https://github.com/pterodactyl/panel/issues/2359
      */
-    public function testCorrectSubuserIsDeletedFromServer()
+    #[TestWith([null])]
+    #[TestWith(['18180000'])]
+    public function testCorrectSubuserIsDeletedFromServer(?string $prefix)
     {
-        $this->swap(DaemonServerRepository::class, $mock = \Mockery::mock(DaemonServerRepository::class));
-
         [$user, $server] = $this->generateTestAccount();
 
         /** @var User $differentUser */
         $differentUser = User::factory()->create();
 
         $real = Uuid::uuid4()->toString();
         // Generate a UUID that lines up with a user in the database if it were to be cast to an int.
-        $uuid = $differentUser->id . substr($real, strlen((string) $differentUser->id));
+        $uuid = ($prefix ?: $differentUser->id) . substr($real, strlen($prefix ?: (string) $differentUser->id));
 
         /** @var User $subuser */
         $subuser = User::factory()->create(['uuid' => $uuid]);
@@ -44,24 +46,18 @@ public function testCorrectSubuserIsDeletedFromServer()
             'permissions' => [Permission::ACTION_WEBSOCKET_CONNECT],
         ]);
 
-        $mock->expects('setServer->revokeUserJTI')->with($subuser->id)->andReturnUndefined();
-
-        $this->actingAs($user)->deleteJson($this->link($server) . "/users/$subuser->uuid")->assertNoContent();
-
-        // Try the same test, but this time with a UUID that if cast to an int (shouldn't) line up with
-        // anything in the database.
-        $uuid = '18180000' . substr(Uuid::uuid4()->toString(), 8);
-        /** @var User $subuser */
-        $subuser = User::factory()->create(['uuid' => $uuid]);
-
-        Subuser::query()->forceCreate([
-            'user_id' => $subuser->id,
-            'server_id' => $server->id,
-            'permissions' => [Permission::ACTION_WEBSOCKET_CONNECT],
-        ]);
+        $this->mock(DaemonRevocationRepository::class, function (MockInterface $mock) use ($subuser, $server) {
+            $mock->expects('setNode')
+                ->with(\Mockery::on(fn ($value) => $value->is($server->node)))
+                ->andReturnSelf();
 
-        $mock->expects('setServer->revokeUserJTI')->with($subuser->id)->andReturnUndefined();
+            $mock->expects('deauthorize')
+                ->with($subuser->uuid, [$server->uuid])
+                ->andReturnUndefined();
+        });
 
-        $this->actingAs($user)->deleteJson($this->link($server) . "/users/$subuser->uuid")->assertNoContent();
+        $this->withoutExceptionHandling()
+            ->actingAs($user)
+            ->deleteJson($this->link($server) . "/users/$subuser->uuid")->assertNoContent();
     }
 }
diff --git a/tests/Integration/Api/Client/Server/Subuser/SubuserAuthorizationTest.php b/tests/Integration/Api/Client/Server/Subuser/SubuserAuthorizationTest.php
@@ -2,9 +2,10 @@
 
 namespace Pterodactyl\Tests\Integration\Api\Client\Server\Subuser;
 
+use Mockery\MockInterface;
 use Pterodactyl\Models\User;
 use Pterodactyl\Models\Subuser;
-use Pterodactyl\Repositories\Wings\DaemonServerRepository;
+use Pterodactyl\Repositories\Wings\DaemonRevocationRepository;
 use Pterodactyl\Tests\Integration\Api\Client\ClientApiIntegrationTestCase;
 
 class SubuserAuthorizationTest extends ClientApiIntegrationTestCase
@@ -34,10 +35,9 @@ public function testUserCannotAccessResourceBelongingToOtherServers(string $meth
         Subuser::factory()->create(['server_id' => $server2->id, 'user_id' => $internal->id]);
         Subuser::factory()->create(['server_id' => $server3->id, 'user_id' => $internal->id]);
 
-        $this->instance(DaemonServerRepository::class, $mock = \Mockery::mock(DaemonServerRepository::class));
-        if ($method === 'DELETE') {
-            $mock->expects('setServer->revokeUserJTI')->with($internal->id)->andReturnUndefined();
-        }
+        $this->mock(DaemonRevocationRepository::class, function (MockInterface $mock) use ($method) {
+            $mock->expects('setNode->deauthorize')->times($method === 'DELETE' ? 1 : 0)->andReturnUndefined();
+        });
 
         // This route is acceptable since they're accessing a subuser on their own server.
         $this->actingAs($user)->json($method, $this->link($server1, '/users/' . $internal->uuid))->assertStatus($method === 'POST' ? 422 : ($method === 'DELETE' ? 204 : 200));
