From b1f6058e31913f03906fe3df402e4ce7bdc2885f Mon Sep 17 00:00:00 2001 From: Dane Everitt Date: Sun, 5 Nov 2017 16:07:50 -0600 Subject: [PATCH] Fix daemon key provider service Handles missing keys if user is an admin or the server owner. Step in the right direction for #733 where all users have their own keys now. Still need to address admin status revocation in order to fully address that issue. --- .../DaemonKeyRepositoryInterface.php | 11 +++ .../Repository/SubuserRepositoryInterface.php | 2 +- .../Eloquent/DaemonKeyRepository.php | 20 ++++ .../Eloquent/SubuserRepository.php | 13 ++- .../DaemonKeys/DaemonKeyCreationService.php | 6 +- .../DaemonKeys/DaemonKeyProviderService.php | 38 ++++++-- app/Transformers/Daemon/ApiKeyTransformer.php | 29 +++--- .../DaemonKeyProviderServiceTest.php | 93 ++++++++++++++----- 8 files changed, 159 insertions(+), 53 deletions(-) diff --git a/app/Contracts/Repository/DaemonKeyRepositoryInterface.php b/app/Contracts/Repository/DaemonKeyRepositoryInterface.php index 154dcb353..572f18c24 100644 --- a/app/Contracts/Repository/DaemonKeyRepositoryInterface.php +++ b/app/Contracts/Repository/DaemonKeyRepositoryInterface.php @@ -24,6 +24,8 @@ namespace Pterodactyl\Contracts\Repository; +use Pterodactyl\Models\DaemonKey; + interface DaemonKeyRepositoryInterface extends RepositoryInterface { /** @@ -31,6 +33,15 @@ interface DaemonKeyRepositoryInterface extends RepositoryInterface */ const INTERNAL_KEY_IDENTIFIER = 'i_'; + /** + * Load the server and user relations onto a key model. + * + * @param \Pterodactyl\Models\DaemonKey $key + * @param bool $refresh + * @return \Pterodactyl\Models\DaemonKey + */ + public function loadServerAndUserRelations(DaemonKey $key, bool $refresh = false): DaemonKey; + /** * Gets the daemon keys associated with a specific server. * diff --git a/app/Contracts/Repository/SubuserRepositoryInterface.php b/app/Contracts/Repository/SubuserRepositoryInterface.php index 8d913a259..06a9efc38 100644 --- a/app/Contracts/Repository/SubuserRepositoryInterface.php +++ b/app/Contracts/Repository/SubuserRepositoryInterface.php @@ -33,7 +33,7 @@ interface SubuserRepositoryInterface extends RepositoryInterface * * @throws \Pterodactyl\Exceptions\Repository\RecordNotFoundException */ - public function getWithPermissionsUsingUserAndServer($user, $server); + public function getWithPermissionsUsingUserAndServer(int $user, int $server): Subuser; /** * Find a subuser and return with server and permissions relationships. diff --git a/app/Repositories/Eloquent/DaemonKeyRepository.php b/app/Repositories/Eloquent/DaemonKeyRepository.php index 238615f72..533128f46 100644 --- a/app/Repositories/Eloquent/DaemonKeyRepository.php +++ b/app/Repositories/Eloquent/DaemonKeyRepository.php @@ -39,6 +39,26 @@ class DaemonKeyRepository extends EloquentRepository implements DaemonKeyReposit return DaemonKey::class; } + /** + * Load the server and user relations onto a key model. + * + * @param \Pterodactyl\Models\DaemonKey $key + * @param bool $refresh + * @return \Pterodactyl\Models\DaemonKey + */ + public function loadServerAndUserRelations(DaemonKey $key, bool $refresh = false): DaemonKey + { + if (! $key->relationLoaded('server') || $refresh) { + $key->load('server'); + } + + if (! $key->relationLoaded('user') || $refresh) { + $key->load('user'); + } + + return $key; + } + /** * {@inheritdoc} */ diff --git a/app/Repositories/Eloquent/SubuserRepository.php b/app/Repositories/Eloquent/SubuserRepository.php index 1b89db8e8..e1387b530 100644 --- a/app/Repositories/Eloquent/SubuserRepository.php +++ b/app/Repositories/Eloquent/SubuserRepository.php @@ -65,13 +65,16 @@ class SubuserRepository extends EloquentRepository implements SubuserRepositoryI } /** - * {@inheritdoc} + * Return a subuser and associated permissions given a user_id and server_id. + * + * @param int $user + * @param int $server + * @return \Pterodactyl\Models\Subuser + * + * @throws \Pterodactyl\Exceptions\Repository\RecordNotFoundException */ - public function getWithPermissionsUsingUserAndServer($user, $server) + public function getWithPermissionsUsingUserAndServer(int $user, int $server): Subuser { - Assert::integerish($user, 'First argument passed to getWithPermissionsUsingUserAndServer must be integer, received %s.'); - Assert::integerish($server, 'Second argument passed to getWithPermissionsUsingUserAndServer must be integer, received %s.'); - $instance = $this->getBuilder()->with('permissions')->where([ ['user_id', '=', $user], ['server_id', '=', $server], diff --git a/app/Services/DaemonKeys/DaemonKeyCreationService.php b/app/Services/DaemonKeys/DaemonKeyCreationService.php index f2b8db990..b9431d28f 100644 --- a/app/Services/DaemonKeys/DaemonKeyCreationService.php +++ b/app/Services/DaemonKeys/DaemonKeyCreationService.php @@ -25,7 +25,6 @@ namespace Pterodactyl\Services\DaemonKeys; use Carbon\Carbon; -use Webmozart\Assert\Assert; use Illuminate\Contracts\Config\Repository as ConfigRepository; use Pterodactyl\Contracts\Repository\DaemonKeyRepositoryInterface; @@ -72,11 +71,8 @@ class DaemonKeyCreationService * * @throws \Pterodactyl\Exceptions\Model\DataValidationException */ - public function handle($server, $user) + public function handle(int $server, int $user) { - Assert::integerish($server, 'First argument passed to handle must be an integer, received %s.'); - Assert::integerish($user, 'Second argument passed to handle must be an integer, received %s.'); - $secret = DaemonKeyRepositoryInterface::INTERNAL_KEY_IDENTIFIER . str_random(40); $this->repository->withoutFresh()->create([ diff --git a/app/Services/DaemonKeys/DaemonKeyProviderService.php b/app/Services/DaemonKeys/DaemonKeyProviderService.php index 2d4ae2b2f..ab19329fa 100644 --- a/app/Services/DaemonKeys/DaemonKeyProviderService.php +++ b/app/Services/DaemonKeys/DaemonKeyProviderService.php @@ -27,10 +27,16 @@ namespace Pterodactyl\Services\DaemonKeys; use Carbon\Carbon; use Pterodactyl\Models\User; use Pterodactyl\Models\Server; +use Pterodactyl\Exceptions\Repository\RecordNotFoundException; use Pterodactyl\Contracts\Repository\DaemonKeyRepositoryInterface; class DaemonKeyProviderService { + /** + * @var \Pterodactyl\Services\DaemonKeys\DaemonKeyCreationService + */ + private $keyCreationService; + /** * @var \Pterodactyl\Services\DaemonKeys\DaemonKeyUpdateService */ @@ -44,11 +50,16 @@ class DaemonKeyProviderService /** * GetDaemonKeyService constructor. * - * @param \Pterodactyl\Services\DaemonKeys\DaemonKeyUpdateService $keyUpdateService + * @param \Pterodactyl\Services\DaemonKeys\DaemonKeyCreationService $keyCreationService * @param \Pterodactyl\Contracts\Repository\DaemonKeyRepositoryInterface $repository + * @param \Pterodactyl\Services\DaemonKeys\DaemonKeyUpdateService $keyUpdateService */ - public function __construct(DaemonKeyUpdateService $keyUpdateService, DaemonKeyRepositoryInterface $repository) - { + public function __construct( + DaemonKeyCreationService $keyCreationService, + DaemonKeyRepositoryInterface $repository, + DaemonKeyUpdateService $keyUpdateService + ) { + $this->keyCreationService = $keyCreationService; $this->keyUpdateService = $keyUpdateService; $this->repository = $repository; } @@ -66,12 +77,23 @@ class DaemonKeyProviderService */ public function handle(Server $server, User $user, $updateIfExpired = true): string { - $userId = $user->root_admin ? $server->owner_id : $user->id; + try { + $key = $this->repository->findFirstWhere([ + ['user_id', '=', $user->id], + ['server_id', '=', $server->id], + ]); + } catch (RecordNotFoundException $exception) { + // If key doesn't exist but we are an admin or the server owner, + // create it. + if ($user->root_admin || $user->id === $server->owner_id) { + return $this->keyCreationService->handle($server->id, $user->id); + } - $key = $this->repository->findFirstWhere([ - ['user_id', '=', $userId], - ['server_id', '=', $server->id], - ]); + // If they aren't the admin or owner of the server, they shouldn't get access. + // Subusers should always have an entry created when they are, so if there is + // no record, it should fail. + throw $exception; + } if (! $updateIfExpired || Carbon::now()->diffInSeconds($key->expires_at, false) > 0) { return $key->secret; diff --git a/app/Transformers/Daemon/ApiKeyTransformer.php b/app/Transformers/Daemon/ApiKeyTransformer.php index a0bae9c5d..e24bf7331 100644 --- a/app/Transformers/Daemon/ApiKeyTransformer.php +++ b/app/Transformers/Daemon/ApiKeyTransformer.php @@ -29,29 +29,30 @@ use Pterodactyl\Models\DaemonKey; use Pterodactyl\Models\Permission; use League\Fractal\TransformerAbstract; use Pterodactyl\Contracts\Repository\SubuserRepositoryInterface; +use Pterodactyl\Contracts\Repository\DaemonKeyRepositoryInterface; class ApiKeyTransformer extends TransformerAbstract { /** - * @var \Carbon\Carbon + * @var \Pterodactyl\Contracts\Repository\DaemonKeyRepositoryInterface */ - protected $carbon; + private $keyRepository; /** * @var \Pterodactyl\Contracts\Repository\SubuserRepositoryInterface */ - protected $repository; + private $repository; /** * ApiKeyTransformer constructor. * - * @param \Carbon\Carbon $carbon - * @param \Pterodactyl\Contracts\Repository\SubuserRepositoryInterface $repository + * @param \Pterodactyl\Contracts\Repository\DaemonKeyRepositoryInterface $keyRepository + * @param \Pterodactyl\Contracts\Repository\SubuserRepositoryInterface $repository */ - public function __construct(Carbon $carbon, SubuserRepositoryInterface $repository) + public function __construct(DaemonKeyRepositoryInterface $keyRepository, SubuserRepositoryInterface $repository) { - $this->carbon = $carbon; $this->repository = $repository; + $this->keyRepository = $keyRepository; } /** @@ -64,18 +65,20 @@ class ApiKeyTransformer extends TransformerAbstract */ public function transform(DaemonKey $key) { - if ($key->user_id === $key->server->owner_id) { + $this->keyRepository->loadServerAndUserRelations($key); + + if ($key->user_id === $key->getRelation('server')->owner_id || $key->getRelation('user')->root_admin) { return [ - 'id' => $key->server->uuid, + 'id' => $key->getRelation('server')->uuid, 'is_temporary' => true, - 'expires_in' => max($this->carbon->now()->diffInSeconds($key->expires_at, false), 0), + 'expires_in' => max(Carbon::now()->diffInSeconds($key->expires_at, false), 0), 'permissions' => ['s:*'], ]; } $subuser = $this->repository->getWithPermissionsUsingUserAndServer($key->user_id, $key->server_id); - $permissions = $subuser->permissions->pluck('permission')->toArray(); + $permissions = $subuser->getRelation('permissions')->pluck('permission')->toArray(); $mappings = Permission::getPermissions(true); $daemonPermissions = []; @@ -86,9 +89,9 @@ class ApiKeyTransformer extends TransformerAbstract } return [ - 'id' => $key->server->uuid, + 'id' => $key->getRelation('server')->uuid, 'is_temporary' => true, - 'expires_in' => max($this->carbon->now()->diffInSeconds($key->expires_at, false), 0), + 'expires_in' => max(Carbon::now()->diffInSeconds($key->expires_at, false), 0), 'permissions' => $daemonPermissions, ]; } diff --git a/tests/Unit/Services/DaemonKeys/DaemonKeyProviderServiceTest.php b/tests/Unit/Services/DaemonKeys/DaemonKeyProviderServiceTest.php index ec2273148..7c240b083 100644 --- a/tests/Unit/Services/DaemonKeys/DaemonKeyProviderServiceTest.php +++ b/tests/Unit/Services/DaemonKeys/DaemonKeyProviderServiceTest.php @@ -16,11 +16,18 @@ use Pterodactyl\Models\User; use Pterodactyl\Models\Server; use Pterodactyl\Models\DaemonKey; use Pterodactyl\Services\DaemonKeys\DaemonKeyUpdateService; +use Pterodactyl\Services\DaemonKeys\DaemonKeyCreationService; use Pterodactyl\Services\DaemonKeys\DaemonKeyProviderService; +use Pterodactyl\Exceptions\Repository\RecordNotFoundException; use Pterodactyl\Contracts\Repository\DaemonKeyRepositoryInterface; class DaemonKeyProviderServiceTest extends TestCase { + /** + * @var \Pterodactyl\Services\DaemonKeys\DaemonKeyCreationService|\Mockery\Mock + */ + private $keyCreationService; + /** * @var \Pterodactyl\Services\DaemonKeys\DaemonKeyUpdateService|\Mockery\Mock */ @@ -39,6 +46,7 @@ class DaemonKeyProviderServiceTest extends TestCase parent::setUp(); Carbon::setTestNow(); + $this->keyCreationService = m::mock(DaemonKeyCreationService::class); $this->keyUpdateService = m::mock(DaemonKeyUpdateService::class); $this->repository = m::mock(DaemonKeyRepositoryInterface::class); } @@ -49,7 +57,7 @@ class DaemonKeyProviderServiceTest extends TestCase public function testKeyIsReturned() { $server = factory(Server::class)->make(); - $user = factory(User::class)->make(['root_admin' => 0]); + $user = factory(User::class)->make(); $key = factory(DaemonKey::class)->make(); $this->repository->shouldReceive('findFirstWhere')->with([ @@ -62,25 +70,6 @@ class DaemonKeyProviderServiceTest extends TestCase $this->assertEquals($key->secret, $response); } - /** - * Test that an admin user gets the server owner's key as the response. - */ - public function testServerOwnerKeyIsReturnedIfUserIsAdministrator() - { - $server = factory(Server::class)->make(); - $user = factory(User::class)->make(['root_admin' => 1]); - $key = factory(DaemonKey::class)->make(); - - $this->repository->shouldReceive('findFirstWhere')->with([ - ['user_id', '=', $server->owner_id], - ['server_id', '=', $server->id], - ])->once()->andReturn($key); - - $response = $this->getService()->handle($server, $user); - $this->assertNotEmpty($response); - $this->assertEquals($key->secret, $response); - } - /** * Test that an expired key is updated and then returned. */ @@ -121,6 +110,68 @@ class DaemonKeyProviderServiceTest extends TestCase $this->assertEquals($key->secret, $response); } + /** + * Test that a key is created if it is missing and the user is a + * root administrator. + */ + public function testMissingKeyIsCreatedIfRootAdmin() + { + $server = factory(Server::class)->make(); + $user = factory(User::class)->make(['root_admin' => 1]); + $key = factory(DaemonKey::class)->make(['expires_at' => Carbon::now()->subHour()]); + + $this->repository->shouldReceive('findFirstWhere')->with([ + ['user_id', '=', $user->id], + ['server_id', '=', $server->id], + ])->once()->andThrow(new RecordNotFoundException); + + $this->keyCreationService->shouldReceive('handle')->with($server->id, $user->id)->once()->andReturn($key->secret); + + $response = $this->getService()->handle($server, $user, false); + $this->assertNotEmpty($response); + $this->assertEquals($key->secret, $response); + } + + /** + * Test that a key is created if it is missing and the user is the + * server owner. + */ + public function testMissingKeyIsCreatedIfUserIsServerOwner() + { + $user = factory(User::class)->make(['root_admin' => 0]); + $server = factory(Server::class)->make(['owner_id' => $user->id]); + $key = factory(DaemonKey::class)->make(['expires_at' => Carbon::now()->subHour()]); + + $this->repository->shouldReceive('findFirstWhere')->with([ + ['user_id', '=', $user->id], + ['server_id', '=', $server->id], + ])->once()->andThrow(new RecordNotFoundException); + + $this->keyCreationService->shouldReceive('handle')->with($server->id, $user->id)->once()->andReturn($key->secret); + + $response = $this->getService()->handle($server, $user, false); + $this->assertNotEmpty($response); + $this->assertEquals($key->secret, $response); + } + + /** + * Test that an exception is thrown if the user should not get a key. + * + * @expectedException \Pterodactyl\Exceptions\Repository\RecordNotFoundException + */ + public function testExceptionIsThrownIfUserDoesNotDeserveKey() + { + $server = factory(Server::class)->make(); + $user = factory(User::class)->make(['root_admin' => 0]); + + $this->repository->shouldReceive('findFirstWhere')->with([ + ['user_id', '=', $user->id], + ['server_id', '=', $server->id], + ])->once()->andThrow(new RecordNotFoundException); + + $this->getService()->handle($server, $user, false); + } + /** * Return an instance of the service with mocked dependencies. * @@ -128,6 +179,6 @@ class DaemonKeyProviderServiceTest extends TestCase */ private function getService(): DaemonKeyProviderService { - return new DaemonKeyProviderService($this->keyUpdateService, $this->repository); + return new DaemonKeyProviderService($this->keyCreationService, $this->repository, $this->keyUpdateService); } }