From a4f03f5d022851da51a55e736af474a088ced850 Mon Sep 17 00:00:00 2001 From: Dane Everitt Date: Sat, 3 Mar 2018 21:31:44 -0600 Subject: [PATCH] Handle missing daemon keys better and fix subuser missing key errors --- CHANGELOG.md | 2 + .../Server/AuthenticateAsSubuser.php | 11 +---- .../DaemonKeys/DaemonKeyProviderService.php | 27 +++++++++-- app/Transformers/Daemon/ApiKeyTransformer.php | 26 +--------- .../Server/AuthenticateAsSubuserTest.php | 10 +--- .../DaemonKeyProviderServiceTest.php | 47 ++++++++++++++++++- 6 files changed, 74 insertions(+), 49 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cbed7fc5..7ee5c4c3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,10 +8,12 @@ This project follows [Semantic Versioning](http://semver.org) guidelines. * Fixes application API keys being created as a client API key. * Search term is now passed through when using paginated result sets. * Reduces the number of SQL queries executed when rendering the server listing to increase performance. +* Fixes exceptions being thrown for non-existent subuser permissions. ### Changed * Databases are now properly paginated when viewing a database host. * No more loading daemon keys for every server model being loaded, some of us value our databases. +* Changed behavior of the subuser middleware to add a daemon access key if one is missing from the database for some reason. ## v0.7.4-h1 (Derelict Dermodactylus) ### Fixed diff --git a/app/Http/Middleware/Server/AuthenticateAsSubuser.php b/app/Http/Middleware/Server/AuthenticateAsSubuser.php index 8f8a158c..1a185bb6 100644 --- a/app/Http/Middleware/Server/AuthenticateAsSubuser.php +++ b/app/Http/Middleware/Server/AuthenticateAsSubuser.php @@ -11,7 +11,6 @@ namespace Pterodactyl\Http\Middleware\Server; use Closure; use Illuminate\Http\Request; -use Illuminate\Contracts\Session\Session; use Pterodactyl\Services\DaemonKeys\DaemonKeyProviderService; use Pterodactyl\Exceptions\Repository\RecordNotFoundException; use Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException; @@ -23,21 +22,14 @@ class AuthenticateAsSubuser */ private $keyProviderService; - /** - * @var \Illuminate\Contracts\Session\Session - */ - private $session; - /** * SubuserAccessAuthenticate constructor. * * @param \Pterodactyl\Services\DaemonKeys\DaemonKeyProviderService $keyProviderService - * @param \Illuminate\Contracts\Session\Session $session */ - public function __construct(DaemonKeyProviderService $keyProviderService, Session $session) + public function __construct(DaemonKeyProviderService $keyProviderService) { $this->keyProviderService = $keyProviderService; - $this->session = $session; } /** @@ -60,7 +52,6 @@ class AuthenticateAsSubuser throw new AccessDeniedHttpException('This account does not have permission to access this server.'); } - $this->session->now('server_data.token', $token); $request->attributes->set('server_token', $token); return $next($request); diff --git a/app/Services/DaemonKeys/DaemonKeyProviderService.php b/app/Services/DaemonKeys/DaemonKeyProviderService.php index ab19329f..a386d286 100644 --- a/app/Services/DaemonKeys/DaemonKeyProviderService.php +++ b/app/Services/DaemonKeys/DaemonKeyProviderService.php @@ -28,6 +28,7 @@ use Carbon\Carbon; use Pterodactyl\Models\User; use Pterodactyl\Models\Server; use Pterodactyl\Exceptions\Repository\RecordNotFoundException; +use Pterodactyl\Contracts\Repository\SubuserRepositoryInterface; use Pterodactyl\Contracts\Repository\DaemonKeyRepositoryInterface; class DaemonKeyProviderService @@ -47,21 +48,29 @@ class DaemonKeyProviderService */ private $repository; + /** + * @var \Pterodactyl\Contracts\Repository\SubuserRepositoryInterface + */ + private $subuserRepository; + /** * GetDaemonKeyService constructor. * * @param \Pterodactyl\Services\DaemonKeys\DaemonKeyCreationService $keyCreationService * @param \Pterodactyl\Contracts\Repository\DaemonKeyRepositoryInterface $repository * @param \Pterodactyl\Services\DaemonKeys\DaemonKeyUpdateService $keyUpdateService + * @param \Pterodactyl\Contracts\Repository\SubuserRepositoryInterface $subuserRepository */ public function __construct( DaemonKeyCreationService $keyCreationService, DaemonKeyRepositoryInterface $repository, - DaemonKeyUpdateService $keyUpdateService + DaemonKeyUpdateService $keyUpdateService, + SubuserRepositoryInterface $subuserRepository ) { $this->keyCreationService = $keyCreationService; $this->keyUpdateService = $keyUpdateService; $this->repository = $repository; + $this->subuserRepository = $subuserRepository; } /** @@ -89,10 +98,18 @@ class DaemonKeyProviderService return $this->keyCreationService->handle($server->id, $user->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; + // Check if user is a subuser for this server. Ideally they should always have + // a record associated with them in the database, but we should still handle + // that potentiality here. + // + // If no subuser is found, a RecordNotFoundException will be thrown, thus handling + // the parent error as well. + $subuser = $this->subuserRepository->findFirstWhere([ + ['user_id', '=', $user->id], + ['server_id', '=', $server->id], + ]); + + return $this->keyCreationService->handle($subuser->server_id, $subuser->user_id); } if (! $updateIfExpired || Carbon::now()->diffInSeconds($key->expires_at, false) > 0) { diff --git a/app/Transformers/Daemon/ApiKeyTransformer.php b/app/Transformers/Daemon/ApiKeyTransformer.php index 54aa37d6..7f1d7427 100644 --- a/app/Transformers/Daemon/ApiKeyTransformer.php +++ b/app/Transformers/Daemon/ApiKeyTransformer.php @@ -1,26 +1,4 @@ . - * - * Permission is hereby granted, free of charge, to any person obtaining a copy - * of this software and associated documentation files (the "Software"), to deal - * in the Software without restriction, including without limitation the rights - * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell - * copies of the Software, and to permit persons to whom the Software is - * furnished to do so, subject to the following conditions: - * - * The above copyright notice and this permission notice shall be included in all - * copies or substantial portions of the Software. - * - * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR - * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, - * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE - * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER - * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, - * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE - * SOFTWARE. - */ namespace Pterodactyl\Transformers\Daemon; @@ -83,8 +61,8 @@ class ApiKeyTransformer extends TransformerAbstract $daemonPermissions = ['s:console']; foreach ($permissions as $permission) { - if (! is_null($mappings[$permission])) { - $daemonPermissions[] = $mappings[$permission]; + if (! is_null(array_get($mappings, $permission))) { + $daemonPermissions[] = array_get($mappings, $permission); } } diff --git a/tests/Unit/Http/Middleware/Server/AuthenticateAsSubuserTest.php b/tests/Unit/Http/Middleware/Server/AuthenticateAsSubuserTest.php index 8ee1a0e4..04965fac 100644 --- a/tests/Unit/Http/Middleware/Server/AuthenticateAsSubuserTest.php +++ b/tests/Unit/Http/Middleware/Server/AuthenticateAsSubuserTest.php @@ -4,7 +4,6 @@ namespace Tests\Unit\Http\Middleware\Server; use Mockery as m; use Pterodactyl\Models\Server; -use Illuminate\Contracts\Session\Session; use Tests\Unit\Http\Middleware\MiddlewareTestCase; use Pterodactyl\Http\Middleware\Server\AuthenticateAsSubuser; use Pterodactyl\Services\DaemonKeys\DaemonKeyProviderService; @@ -17,11 +16,6 @@ class AuthenticateAsSubuserTest extends MiddlewareTestCase */ private $keyProviderService; - /** - * @var \Illuminate\Contracts\Session\Session|\Mockery\Mock - */ - private $session; - /** * Setup tests. */ @@ -30,7 +24,6 @@ class AuthenticateAsSubuserTest extends MiddlewareTestCase parent::setUp(); $this->keyProviderService = m::mock(DaemonKeyProviderService::class); - $this->session = m::mock(Session::class); } /** @@ -43,7 +36,6 @@ class AuthenticateAsSubuserTest extends MiddlewareTestCase $this->setRequestAttribute('server', $model); $this->keyProviderService->shouldReceive('handle')->with($model, $user)->once()->andReturn('abc123'); - $this->session->shouldReceive('now')->with('server_data.token', 'abc123')->once()->andReturnNull(); $this->getMiddleware()->handle($this->request, $this->getClosureAssertions()); $this->assertRequestHasAttribute('server_token'); @@ -74,6 +66,6 @@ class AuthenticateAsSubuserTest extends MiddlewareTestCase */ public function getMiddleware(): AuthenticateAsSubuser { - return new AuthenticateAsSubuser($this->keyProviderService, $this->session); + return new AuthenticateAsSubuser($this->keyProviderService); } } diff --git a/tests/Unit/Services/DaemonKeys/DaemonKeyProviderServiceTest.php b/tests/Unit/Services/DaemonKeys/DaemonKeyProviderServiceTest.php index 87d5f506..81d1b3a0 100644 --- a/tests/Unit/Services/DaemonKeys/DaemonKeyProviderServiceTest.php +++ b/tests/Unit/Services/DaemonKeys/DaemonKeyProviderServiceTest.php @@ -14,11 +14,13 @@ use Carbon\Carbon; use Tests\TestCase; use Pterodactyl\Models\User; use Pterodactyl\Models\Server; +use Pterodactyl\Models\Subuser; 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\SubuserRepositoryInterface; use Pterodactyl\Contracts\Repository\DaemonKeyRepositoryInterface; class DaemonKeyProviderServiceTest extends TestCase @@ -38,6 +40,11 @@ class DaemonKeyProviderServiceTest extends TestCase */ private $repository; + /** + * @var \Pterodactyl\Contracts\Repository\SubuserRepositoryInterface|\Mockery\Mock + */ + private $subuserRepository; + /** * Setup tests. */ @@ -49,6 +56,7 @@ class DaemonKeyProviderServiceTest extends TestCase $this->keyCreationService = m::mock(DaemonKeyCreationService::class); $this->keyUpdateService = m::mock(DaemonKeyUpdateService::class); $this->repository = m::mock(DaemonKeyRepositoryInterface::class); + $this->subuserRepository = m::mock(SubuserRepositoryInterface::class); } /** @@ -154,6 +162,33 @@ class DaemonKeyProviderServiceTest extends TestCase $this->assertEquals($key->secret, $response); } + /** + * Test that a missing key is created for a subuser. + */ + public function testMissingKeyIsCreatedForSubuser() + { + $user = factory(User::class)->make(['root_admin' => 0]); + $server = factory(Server::class)->make(); + $key = factory(DaemonKey::class)->make(['expires_at' => Carbon::now()->subHour()]); + $subuser = factory(Subuser::class)->make(['user_id' => $user->id, 'server_id' => $server->id]); + + $this->repository->shouldReceive('findFirstWhere')->with([ + ['user_id', '=', $user->id], + ['server_id', '=', $server->id], + ])->once()->andThrow(new RecordNotFoundException); + + $this->subuserRepository->shouldReceive('findFirstWhere')->once()->with([ + ['user_id', '=', $user->id], + ['server_id', '=', $server->id], + ])->andReturn($subuser); + + $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. * @@ -169,6 +204,11 @@ class DaemonKeyProviderServiceTest extends TestCase ['server_id', '=', $server->id], ])->once()->andThrow(new RecordNotFoundException); + $this->subuserRepository->shouldReceive('findFirstWhere')->once()->with([ + ['user_id', '=', $user->id], + ['server_id', '=', $server->id], + ])->andThrow(new RecordNotFoundException); + $this->getService()->handle($server, $user, false); } @@ -179,6 +219,11 @@ class DaemonKeyProviderServiceTest extends TestCase */ private function getService(): DaemonKeyProviderService { - return new DaemonKeyProviderService($this->keyCreationService, $this->repository, $this->keyUpdateService); + return new DaemonKeyProviderService( + $this->keyCreationService, + $this->repository, + $this->keyUpdateService, + $this->subuserRepository + ); } }