From e9c633fd03222968d175312a90122c123a74374b Mon Sep 17 00:00:00 2001 From: DaneEveritt Date: Sun, 22 May 2022 15:37:39 -0400 Subject: [PATCH] Update transformers and controllers to no longer pull an API key attribute --- .../Application/ApplicationApiController.php | 17 +++-- .../Api/Client/ClientApiController.php | 25 ++++--- .../Api/Application/ApplicationApiRequest.php | 16 ++-- .../Api/Application/BaseTransformer.php | 66 +++++++++-------- .../Api/Client/BaseClientTransformer.php | 40 ++-------- .../Api/Client/DatabaseTransformer.php | 2 +- .../Api/Client/ServerTransformer.php | 15 ++-- .../ApplicationApiIntegrationTestCase.php | 10 ++- .../Api/AuthenticateIPAccessTest.php | 73 ------------------- 9 files changed, 91 insertions(+), 173 deletions(-) delete mode 100644 tests/Unit/Http/Middleware/Api/AuthenticateIPAccessTest.php diff --git a/app/Http/Controllers/Api/Application/ApplicationApiController.php b/app/Http/Controllers/Api/Application/ApplicationApiController.php index 6078b6f2c..91da81893 100644 --- a/app/Http/Controllers/Api/Application/ApplicationApiController.php +++ b/app/Http/Controllers/Api/Application/ApplicationApiController.php @@ -55,17 +55,20 @@ abstract class ApplicationApiController extends Controller /** * Return an instance of an application transformer. * - * @return \Pterodactyl\Transformers\Api\Application\BaseTransformer + * @template T of \Pterodactyl\Transformers\Api\Application\BaseTransformer + * + * @param class-string $abstract + * + * @return T + * + * @noinspection PhpDocSignatureInspection + * @noinspection PhpUndefinedClassInspection */ public function getTransformer(string $abstract) { - /** @var \Pterodactyl\Transformers\Api\Application\BaseTransformer $transformer */ - $transformer = Container::getInstance()->make($abstract); - $transformer->setKey($this->request->attributes->get('api_key')); + Assert::subclassOf($abstract, BaseTransformer::class); - Assert::isInstanceOf($transformer, BaseTransformer::class); - - return $transformer; + return $abstract::fromRequest($this->request); } /** diff --git a/app/Http/Controllers/Api/Client/ClientApiController.php b/app/Http/Controllers/Api/Client/ClientApiController.php index c7f6f1a49..210489dea 100644 --- a/app/Http/Controllers/Api/Client/ClientApiController.php +++ b/app/Http/Controllers/Api/Client/ClientApiController.php @@ -3,7 +3,6 @@ namespace Pterodactyl\Http\Controllers\Api\Client; use Webmozart\Assert\Assert; -use Illuminate\Container\Container; use Pterodactyl\Transformers\Daemon\BaseDaemonTransformer; use Pterodactyl\Transformers\Api\Client\BaseClientTransformer; use Pterodactyl\Http\Controllers\Api\Application\ApplicationApiController; @@ -45,21 +44,23 @@ abstract class ClientApiController extends ApplicationApiController /** * Return an instance of an application transformer. * - * @return \Pterodactyl\Transformers\Api\Client\BaseClientTransformer + * @template T of \Pterodactyl\Transformers\Api\Client\BaseClientTransformer + * + * @param class-string $abstract + * + * @return T + * + * @noinspection PhpUndefinedClassInspection + * @noinspection PhpDocSignatureInspection */ public function getTransformer(string $abstract) { - /** @var \Pterodactyl\Transformers\Api\Client\BaseClientTransformer $transformer */ - $transformer = Container::getInstance()->make($abstract); - Assert::isInstanceOfAny($transformer, [ - BaseClientTransformer::class, - BaseDaemonTransformer::class, - ]); + Assert::methodExists($abstract, 'fromRequest'); - if ($transformer instanceof BaseClientTransformer) { - $transformer->setKey($this->request->attributes->get('api_key')); - $transformer->setUser($this->request->user()); - } + /** @var T $transformer */ + $transformer = $abstract::fromRequest($this->request); + + Assert::isInstanceOfAny($transformer, [BaseClientTransformer::class, BaseDaemonTransformer::class]); return $transformer; } diff --git a/app/Http/Requests/Api/Application/ApplicationApiRequest.php b/app/Http/Requests/Api/Application/ApplicationApiRequest.php index 5dc903f53..ec7643bd5 100644 --- a/app/Http/Requests/Api/Application/ApplicationApiRequest.php +++ b/app/Http/Requests/Api/Application/ApplicationApiRequest.php @@ -3,6 +3,7 @@ namespace Pterodactyl\Http\Requests\Api\Application; use Webmozart\Assert\Assert; +use Laravel\Sanctum\TransientToken; use Illuminate\Validation\Validator; use Illuminate\Database\Eloquent\Model; use Pterodactyl\Services\Acl\Api\AdminAcl; @@ -11,14 +12,6 @@ use Pterodactyl\Exceptions\PterodactylException; abstract class ApplicationApiRequest extends FormRequest { - /** - * Tracks if the request has been validated internally or not to avoid - * making duplicate validation calls. - * - * @var bool - */ - private $hasValidated = false; - /** * The resource that should be checked when performing the authorization * function for this request. @@ -47,7 +40,12 @@ abstract class ApplicationApiRequest extends FormRequest throw new PterodactylException('An ACL resource must be defined on API requests.'); } - return AdminAcl::check($this->attributes->get('api_key'), $this->resource, $this->permission); + $token = $this->user()->currentAccessToken(); + if ($token instanceof TransientToken) { + return true; + } + + return AdminAcl::check($token, $this->resource, $this->permission); } /** diff --git a/app/Transformers/Api/Application/BaseTransformer.php b/app/Transformers/Api/Application/BaseTransformer.php index 3c5630366..bb8f5ce9b 100644 --- a/app/Transformers/Api/Application/BaseTransformer.php +++ b/app/Transformers/Api/Application/BaseTransformer.php @@ -3,12 +3,13 @@ namespace Pterodactyl\Transformers\Api\Application; use Carbon\CarbonImmutable; +use Illuminate\Http\Request; +use Webmozart\Assert\Assert; use Pterodactyl\Models\ApiKey; use Illuminate\Container\Container; use Illuminate\Database\Eloquent\Model; use League\Fractal\TransformerAbstract; use Pterodactyl\Services\Acl\Api\AdminAcl; -use Pterodactyl\Exceptions\Transformer\InvalidTransformerLevelException; /** * @method array transform(Model $model) @@ -17,15 +18,7 @@ abstract class BaseTransformer extends TransformerAbstract { public const RESPONSE_TIMEZONE = 'UTC'; - /** - * @var \Pterodactyl\Models\ApiKey - */ - private $key; - - /** - * Return the resource name for the JSONAPI output. - */ - abstract public function getResourceName(): string; + protected Request $request; /** * BaseTransformer constructor. @@ -39,54 +32,69 @@ abstract class BaseTransformer extends TransformerAbstract } /** - * Set the HTTP request class being used for this request. - * - * @return $this + * Return the resource name for the JSONAPI output. */ - public function setKey(ApiKey $key) + abstract public function getResourceName(): string; + + /** + * Sets the request on the instance. + * + * @return static + */ + public function setRequest(Request $request): self { - $this->key = $key; + $this->request = $request; return $this; } /** - * Return the request instance being used for this transformer. + * Returns a new transformer instance with the request set on the instance. + * + * @return \Pterodactyl\Transformers\Api\Application\BaseTransformer */ - public function getKey(): ApiKey + public static function fromRequest(Request $request) { - return $this->key; + return app(static::class)->setRequest($request); } /** * Determine if the API key loaded onto the transformer has permission * to access a different resource. This is used when including other * models on a transformation request. + * + * @deprecated — prefer $user->can/cannot methods */ protected function authorize(string $resource): bool { - return AdminAcl::check($this->getKey(), $resource, AdminAcl::READ); + $token = $this->request->user()->currentAccessToken(); + if (!$token instanceof ApiKey || $token->key_type !== ApiKey::TYPE_APPLICATION) { + return false; + } + + return AdminAcl::check($token, $resource, AdminAcl::READ); } /** * Create a new instance of the transformer and pass along the currently * set API key. * - * @return \Pterodactyl\Transformers\Api\Application\BaseTransformer + * @template T of \Pterodactyl\Transformers\Api\Application\BaseTransformer + * + * @param class-string $abstract + * + * @return T * * @throws \Pterodactyl\Exceptions\Transformer\InvalidTransformerLevelException + * + * @noinspection PhpUndefinedClassInspection + * @noinspection PhpDocSignatureInspection */ - protected function makeTransformer(string $abstract, array $parameters = []) + protected function makeTransformer(string $abstract) { - /** @var \Pterodactyl\Transformers\Api\Application\BaseTransformer $transformer */ - $transformer = Container::getInstance()->makeWith($abstract, $parameters); - $transformer->setKey($this->getKey()); + Assert::subclassOf($abstract, self::class); - if (!$transformer instanceof self) { - throw new InvalidTransformerLevelException('Calls to ' . __METHOD__ . ' must return a transformer that is an instance of ' . __CLASS__); - } - - return $transformer; + return $abstract::fromRequest($this->request); } /** diff --git a/app/Transformers/Api/Client/BaseClientTransformer.php b/app/Transformers/Api/Client/BaseClientTransformer.php index efe53276f..0388effb0 100644 --- a/app/Transformers/Api/Client/BaseClientTransformer.php +++ b/app/Transformers/Api/Client/BaseClientTransformer.php @@ -5,31 +5,16 @@ namespace Pterodactyl\Transformers\Api\Client; use Pterodactyl\Models\User; use Webmozart\Assert\Assert; use Pterodactyl\Models\Server; -use Illuminate\Container\Container; -use Pterodactyl\Exceptions\Transformer\InvalidTransformerLevelException; use Pterodactyl\Transformers\Api\Application\BaseTransformer as BaseApplicationTransformer; abstract class BaseClientTransformer extends BaseApplicationTransformer { - /** - * @var \Pterodactyl\Models\User - */ - private $user; - /** * Return the user model of the user requesting this transformation. */ public function getUser(): User { - return $this->user; - } - - /** - * Set the user model of the user requesting this transformation. - */ - public function setUser(User $user) - { - $this->user = $user; + return $this->request->user(); } /** @@ -37,33 +22,22 @@ abstract class BaseClientTransformer extends BaseApplicationTransformer * to access a different resource. This is used when including other * models on a transformation request. * - * @param \Pterodactyl\Models\Server $server + * @noinspection PhpParameterNameChangedDuringInheritanceInspection */ protected function authorize(string $ability, Server $server = null): bool { Assert::isInstanceOf($server, Server::class); - return $this->getUser()->can($ability, [$server]); + return $this->request->user()->can($ability, [$server]); } /** - * Create a new instance of the transformer and pass along the currently - * set API key. - * - * @return self - * - * @throws \Pterodactyl\Exceptions\Transformer\InvalidTransformerLevelException + * {@inheritDoc} */ - protected function makeTransformer(string $abstract, array $parameters = []) + protected function makeTransformer(string $abstract) { - /** @var \Pterodactyl\Transformers\Api\Application\BaseTransformer $transformer */ - $transformer = Container::getInstance()->makeWith($abstract, $parameters); - $transformer->setKey($this->getKey()); + Assert::subclassOf($abstract, self::class); - if (!$transformer instanceof self) { - throw new InvalidTransformerLevelException('Calls to ' . __METHOD__ . ' must return a transformer that is an instance of ' . __CLASS__); - } - - return $transformer; + return parent::makeTransformer($abstract); } } diff --git a/app/Transformers/Api/Client/DatabaseTransformer.php b/app/Transformers/Api/Client/DatabaseTransformer.php index 4857ea027..bdabbeed5 100644 --- a/app/Transformers/Api/Client/DatabaseTransformer.php +++ b/app/Transformers/Api/Client/DatabaseTransformer.php @@ -59,7 +59,7 @@ class DatabaseTransformer extends BaseClientTransformer */ public function includePassword(Database $database) { - if (!$this->getUser()->can(Permission::ACTION_DATABASE_VIEW_PASSWORD, $database->server)) { + if (!$this->request->user()->can(Permission::ACTION_DATABASE_VIEW_PASSWORD, $database->server)) { return $this->null(); } diff --git a/app/Transformers/Api/Client/ServerTransformer.php b/app/Transformers/Api/Client/ServerTransformer.php index 583ccab68..53f4adf4b 100644 --- a/app/Transformers/Api/Client/ServerTransformer.php +++ b/app/Transformers/Api/Client/ServerTransformer.php @@ -34,8 +34,10 @@ class ServerTransformer extends BaseClientTransformer /** @var \Pterodactyl\Services\Servers\StartupCommandService $service */ $service = Container::getInstance()->make(StartupCommandService::class); + $user = $this->request->user(); + return [ - 'server_owner' => $this->getKey()->user_id === $server->owner_id, + 'server_owner' => $user->id === $server->owner_id, 'identifier' => $server->uuidShort, 'internal_id' => $server->id, 'uuid' => $server->uuid, @@ -55,7 +57,7 @@ class ServerTransformer extends BaseClientTransformer 'threads' => $server->threads, 'oom_disabled' => $server->oom_disabled, ], - 'invocation' => $service->handle($server, !$this->getUser()->can(Permission::ACTION_STARTUP_READ, $server)), + 'invocation' => $service->handle($server, !$user->can(Permission::ACTION_STARTUP_READ, $server)), 'docker_image' => $server->image, 'egg_features' => $server->egg->inherit_features, 'feature_limits' => [ @@ -75,7 +77,7 @@ class ServerTransformer extends BaseClientTransformer /** * Returns the allocations associated with this server. * - * @return \League\Fractal\Resource\Collection|\League\Fractal\Resource\NullResource + * @return \League\Fractal\Resource\Collection * * @throws \Pterodactyl\Exceptions\Transformer\InvalidTransformerLevelException */ @@ -83,6 +85,7 @@ class ServerTransformer extends BaseClientTransformer { $transformer = $this->makeTransformer(AllocationTransformer::class); + $user = $this->request->user(); // While we include this permission, we do need to actually handle it slightly different here // for the purpose of keeping things functionally working. If the user doesn't have read permissions // for the allocations we'll only return the primary server allocation, and any notes associated @@ -90,7 +93,7 @@ class ServerTransformer extends BaseClientTransformer // // This allows us to avoid too much permission regression, without also hiding information that // is generally needed for the frontend to make sense when browsing or searching results. - if (!$this->getUser()->can(Permission::ACTION_ALLOCATION_READ, $server)) { + if (!$user->can(Permission::ACTION_ALLOCATION_READ, $server)) { $primary = clone $server->allocation; $primary->notes = null; @@ -107,7 +110,7 @@ class ServerTransformer extends BaseClientTransformer */ public function includeVariables(Server $server) { - if (!$this->getUser()->can(Permission::ACTION_STARTUP_READ, $server)) { + if (!$this->request->user()->can(Permission::ACTION_STARTUP_READ, $server)) { return $this->null(); } @@ -139,7 +142,7 @@ class ServerTransformer extends BaseClientTransformer */ public function includeSubusers(Server $server) { - if (!$this->getUser()->can(Permission::ACTION_USER_READ, $server)) { + if (!$this->request->user()->can(Permission::ACTION_USER_READ, $server)) { return $this->null(); } diff --git a/tests/Integration/Api/Application/ApplicationApiIntegrationTestCase.php b/tests/Integration/Api/Application/ApplicationApiIntegrationTestCase.php index daca17283..8220f0252 100644 --- a/tests/Integration/Api/Application/ApplicationApiIntegrationTestCase.php +++ b/tests/Integration/Api/Application/ApplicationApiIntegrationTestCase.php @@ -3,6 +3,7 @@ namespace Pterodactyl\Tests\Integration\Api\Application; use Pterodactyl\Models\User; +use Illuminate\Http\Request; use PHPUnit\Framework\Assert; use Pterodactyl\Models\ApiKey; use Pterodactyl\Services\Acl\Api\AdminAcl; @@ -110,9 +111,12 @@ abstract class ApplicationApiIntegrationTestCase extends IntegrationTestCase */ protected function getTransformer(string $abstract): BaseTransformer { - /** @var \Pterodactyl\Transformers\Api\Application\BaseTransformer $transformer */ - $transformer = $this->app->make($abstract); - $transformer->setKey($this->getApiKey()); + $request = Request::createFromGlobals(); + $request->setUserResolver(function () { + return $this->getApiKey()->user; + }); + + $transformer = $abstract::fromRequest($request); Assert::assertInstanceOf(BaseTransformer::class, $transformer); Assert::assertNotInstanceOf(BaseClientTransformer::class, $transformer); diff --git a/tests/Unit/Http/Middleware/Api/AuthenticateIPAccessTest.php b/tests/Unit/Http/Middleware/Api/AuthenticateIPAccessTest.php deleted file mode 100644 index c362e9cca..000000000 --- a/tests/Unit/Http/Middleware/Api/AuthenticateIPAccessTest.php +++ /dev/null @@ -1,73 +0,0 @@ -make(['allowed_ips' => []]); - $this->setRequestAttribute('api_key', $model); - - $this->getMiddleware()->handle($this->request, $this->getClosureAssertions()); - } - - /** - * Test middleware works correctly when a valid IP accesses - * and there is an IP restriction. - */ - public function testWithValidIP() - { - $model = ApiKey::factory()->make(['allowed_ips' => ['127.0.0.1']]); - $this->setRequestAttribute('api_key', $model); - - $this->request->shouldReceive('ip')->withNoArgs()->once()->andReturn('127.0.0.1'); - - $this->getMiddleware()->handle($this->request, $this->getClosureAssertions()); - } - - /** - * Test that a CIDR range can be used. - */ - public function testValidIPAgainstCIDRRange() - { - $model = ApiKey::factory()->make(['allowed_ips' => ['192.168.1.1/28']]); - $this->setRequestAttribute('api_key', $model); - - $this->request->shouldReceive('ip')->withNoArgs()->once()->andReturn('192.168.1.15'); - - $this->getMiddleware()->handle($this->request, $this->getClosureAssertions()); - } - - /** - * Test that an exception is thrown when an invalid IP address - * tries to connect and there is an IP restriction. - */ - public function testWithInvalidIP() - { - $this->expectException(AccessDeniedHttpException::class); - - $model = ApiKey::factory()->make(['allowed_ips' => ['127.0.0.1']]); - $this->setRequestAttribute('api_key', $model); - - $this->request->shouldReceive('ip')->withNoArgs()->twice()->andReturn('127.0.0.2'); - - $this->getMiddleware()->handle($this->request, $this->getClosureAssertions()); - } - - /** - * Return an instance of the middleware to be used when testing. - */ - private function getMiddleware(): AuthenticateIPAccess - { - return new AuthenticateIPAccess(); - } -}