- Database connection details
+ Database connection details
From 11054de5b32137459113b727e0e0e191351ce906 Mon Sep 17 00:00:00 2001
From: Dane Everitt
Date: Sun, 6 Dec 2020 12:16:12 -0800
Subject: [PATCH 12/14] 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;
+ });
}
}
From 79673ca44075b44e3ea22c0b64373f66510830b2 Mon Sep 17 00:00:00 2001
From: Dane Everitt
Date: Sun, 6 Dec 2020 12:23:58 -0800
Subject: [PATCH 13/14] Don't ever block storing node updates if wings returns
an error; closes #2712
---
app/Services/Nodes/NodeUpdateService.php | 15 +++++++++++----
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/app/Services/Nodes/NodeUpdateService.php b/app/Services/Nodes/NodeUpdateService.php
index ce9238d7c..f684d1064 100644
--- a/app/Services/Nodes/NodeUpdateService.php
+++ b/app/Services/Nodes/NodeUpdateService.php
@@ -4,6 +4,7 @@ namespace Pterodactyl\Services\Nodes;
use Illuminate\Support\Str;
use Pterodactyl\Models\Node;
+use Illuminate\Support\Facades\Log;
use GuzzleHttp\Exception\ConnectException;
use Illuminate\Database\ConnectionInterface;
use Illuminate\Contracts\Encryption\Encrypter;
@@ -90,11 +91,17 @@ class NodeUpdateService
$this->configurationRepository->setNode($node)->update($updated);
} catch (DaemonConnectionException $exception) {
- if (! is_null($exception->getPrevious()) && $exception->getPrevious() instanceof ConnectException) {
- return [$updated, true];
- }
+ Log::warning($exception, ['node_id' => $node->id]);
- throw $exception;
+ // Never actually throw these exceptions up the stack. If we were able to change the settings
+ // but something went wrong with Wings we just want to store the update and let the user manually
+ // make changes as needed.
+ //
+ // This avoids issues with proxies such as CloudFlare which will see Wings as offline and then
+ // inject their own response pages, causing this logic to get fucked up.
+ //
+ // @see https://github.com/pterodactyl/panel/issues/2712
+ return [ $updated, true ];
}
return [$updated, false];
From 5d23d894ae49c1f1e96899c1abd82ba9d94d82a8 Mon Sep 17 00:00:00 2001
From: Dane Everitt
Date: Sun, 6 Dec 2020 12:25:36 -0800
Subject: [PATCH 14/14] Update NodeUpdateServiceTest.php
---
tests/Unit/Services/Nodes/NodeUpdateServiceTest.php | 11 +++--------
1 file changed, 3 insertions(+), 8 deletions(-)
diff --git a/tests/Unit/Services/Nodes/NodeUpdateServiceTest.php b/tests/Unit/Services/Nodes/NodeUpdateServiceTest.php
index c8138185d..1a836e446 100644
--- a/tests/Unit/Services/Nodes/NodeUpdateServiceTest.php
+++ b/tests/Unit/Services/Nodes/NodeUpdateServiceTest.php
@@ -210,11 +210,8 @@ class NodeUpdateServiceTest extends TestCase
try {
$closure();
} catch (Exception $exception) {
- $this->assertInstanceOf(DaemonConnectionException::class, $exception);
- $this->assertSame(
- 'There was an exception while attempting to communicate with the daemon resulting in a HTTP/E_CONN_REFUSED response code. This exception has been logged.',
- $exception->getMessage()
- );
+ $this->assertInstanceOf(Exception::class, $exception);
+ $this->assertSame('Foo', $exception->getMessage());
return true;
}
@@ -224,9 +221,7 @@ class NodeUpdateServiceTest extends TestCase
$this->repository->expects('withFreshModel->update')->andReturns($updatedModel);
$this->configurationRepository->expects('setNode->update')->andThrow(
- new DaemonConnectionException(
- new TransferException('', 500, new Exception)
- )
+ new Exception('Foo')
);
$this->getService()->handle($model, ['name' => $updatedModel->name]);