From 11054de5b32137459113b727e0e0e191351ce906 Mon Sep 17 00:00:00 2001 From: Dane Everitt Date: Sun, 6 Dec 2020 12:16:12 -0800 Subject: [PATCH] Attempt revocation of JWT access when changing a server's owner closes #2771 --- .../Api/Client/Servers/SubuserController.php | 4 +- .../Wings/DaemonServerRepository.php | 18 +++++- .../Servers/DetailsModificationService.php | 56 +++++++++++-------- 3 files changed, 53 insertions(+), 25 deletions(-) diff --git a/app/Http/Controllers/Api/Client/Servers/SubuserController.php b/app/Http/Controllers/Api/Client/Servers/SubuserController.php index d2806a653..81238d33b 100644 --- a/app/Http/Controllers/Api/Client/Servers/SubuserController.php +++ b/app/Http/Controllers/Api/Client/Servers/SubuserController.php @@ -135,7 +135,7 @@ class SubuserController extends ClientApiController ]); try { - $this->serverRepository->setServer($server)->revokeJTIs([md5($subuser->user_id . $server->uuid)]); + $this->serverRepository->setServer($server)->revokeUserJTI($subuser->user_id); } catch (DaemonConnectionException $exception) { // Don't block this request if we can't connect to the Wings instance. Chances are it is // offline in this event and the token will be invalid anyways once Wings boots back. @@ -163,7 +163,7 @@ class SubuserController extends ClientApiController $this->repository->delete($subuser->id); try { - $this->serverRepository->setServer($server)->revokeJTIs([md5($subuser->user_id . $server->uuid)]); + $this->serverRepository->setServer($server)->revokeUserJTI($subuser->user_id); } 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/DaemonServerRepository.php b/app/Repositories/Wings/DaemonServerRepository.php index 22c90d6c5..f8af0c034 100644 --- a/app/Repositories/Wings/DaemonServerRepository.php +++ b/app/Repositories/Wings/DaemonServerRepository.php @@ -3,6 +3,7 @@ namespace Pterodactyl\Repositories\Wings; use Webmozart\Assert\Assert; +use Pterodactyl\Models\User; use Pterodactyl\Models\Server; use GuzzleHttp\Exception\TransferException; use Pterodactyl\Exceptions\Http\Connection\DaemonConnectionException; @@ -144,6 +145,21 @@ class DaemonServerRepository extends DaemonRepository } } + /** + * Revokes a single user's JTI by using their ID. This is simply a helper function to + * 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. + * + * @param int $id + * @throws \Pterodactyl\Exceptions\Http\Connection\DaemonConnectionException + */ + public function revokeUserJTI(int $id): void + { + Assert::isInstanceOf($this->server, Server::class); + + $this->revokeJTIs([ md5($id . $this->server->uuid) ]); + } + /** * Revokes an array of JWT JTI's by marking any token generated before the current time on * the Wings instance as being invalid. @@ -151,7 +167,7 @@ class DaemonServerRepository extends DaemonRepository * @param array $jtis * @throws \Pterodactyl\Exceptions\Http\Connection\DaemonConnectionException */ - public function revokeJTIs(array $jtis): void + protected function revokeJTIs(array $jtis): void { Assert::isInstanceOf($this->server, Server::class); diff --git a/app/Services/Servers/DetailsModificationService.php b/app/Services/Servers/DetailsModificationService.php index 65a5b2815..aa480a06a 100644 --- a/app/Services/Servers/DetailsModificationService.php +++ b/app/Services/Servers/DetailsModificationService.php @@ -2,10 +2,12 @@ namespace Pterodactyl\Services\Servers; +use Illuminate\Support\Arr; use Pterodactyl\Models\Server; use Illuminate\Database\ConnectionInterface; use Pterodactyl\Traits\Services\ReturnsUpdatedModels; -use Pterodactyl\Repositories\Eloquent\ServerRepository; +use Pterodactyl\Repositories\Wings\DaemonServerRepository; +use Pterodactyl\Exceptions\Http\Connection\DaemonConnectionException; class DetailsModificationService { @@ -17,22 +19,20 @@ class DetailsModificationService private $connection; /** - * @var \Pterodactyl\Repositories\Eloquent\ServerRepository + * @var \Pterodactyl\Repositories\Wings\DaemonServerRepository */ - private $repository; + private $serverRepository; /** * DetailsModificationService constructor. * * @param \Illuminate\Database\ConnectionInterface $connection - * @param \Pterodactyl\Repositories\Eloquent\ServerRepository $repository + * @param \Pterodactyl\Repositories\Wings\DaemonServerRepository $serverRepository */ - public function __construct( - ConnectionInterface $connection, - ServerRepository $repository - ) { + public function __construct(ConnectionInterface $connection, DaemonServerRepository $serverRepository) + { $this->connection = $connection; - $this->repository = $repository; + $this->serverRepository = $serverRepository; } /** @@ -40,24 +40,36 @@ class DetailsModificationService * * @param \Pterodactyl\Models\Server $server * @param array $data - * @return bool|\Pterodactyl\Models\Server + * @return \Pterodactyl\Models\Server * - * @throws \Pterodactyl\Exceptions\Model\DataValidationException - * @throws \Pterodactyl\Exceptions\Repository\RecordNotFoundException + * @throws \Throwable */ - public function handle(Server $server, array $data) + public function handle(Server $server, array $data): Server { - $this->connection->beginTransaction(); + return $this->connection->transaction(function () use ($data, $server) { + $owner = $server->owner_id; - $response = $this->repository->setFreshModel($this->getUpdatedModel())->update($server->id, [ - 'external_id' => array_get($data, 'external_id'), - 'owner_id' => array_get($data, 'owner_id'), - 'name' => array_get($data, 'name'), - 'description' => array_get($data, 'description') ?? '', - ], true, true); + $server->forceFill([ + 'external_id' => Arr::get($data, 'external_id'), + 'owner_id' => Arr::get($data, 'owner_id'), + 'name' => Arr::get($data, 'name'), + 'description' => Arr::get($data, 'description') ?? '', + ])->saveOrFail(); - $this->connection->commit(); + // 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) { + try { + $this->serverRepository->setServer($server)->revokeUserJTI($owner); + } 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. Remeber, these tokens reset every + // few minutes by default, we're just trying to help it along a little quicker. + } + } - return $response; + return $server; + }); } }