From 61e977133318a1888d79d2048841660650f250d1 Mon Sep 17 00:00:00 2001 From: Dane Everitt Date: Wed, 19 Aug 2020 20:21:12 -0700 Subject: [PATCH] Code cleanup for subuser API endpoints; closes #2247 --- app/Exceptions/Handler.php | 7 ++ .../Api/Client/Servers/SubuserController.php | 34 ++++++-- .../Client/Server/SubuserBelongsToServer.php | 37 +++++++++ .../Client/SubstituteClientApiBindings.php | 5 ++ .../Servers/Subusers/SubuserRequest.php | 78 +++---------------- app/Models/Server.php | 2 +- .../Eloquent/SubuserRepository.php | 24 ------ .../Servers/GetUserPermissionsService.php | 2 +- routes/api-client.php | 9 ++- 9 files changed, 94 insertions(+), 104 deletions(-) create mode 100644 app/Http/Middleware/Api/Client/Server/SubuserBelongsToServer.php diff --git a/app/Exceptions/Handler.php b/app/Exceptions/Handler.php index 50ac1a960..d278ce0bc 100644 --- a/app/Exceptions/Handler.php +++ b/app/Exceptions/Handler.php @@ -213,6 +213,13 @@ class Handler extends ExceptionHandler 'detail' => 'An error was encountered while processing this request.', ]; + if ($exception instanceof ModelNotFoundException || $exception->getPrevious() instanceof ModelNotFoundException) { + // Show a nicer error message compared to the standard "No query results for model" + // response that is normally returned. If we are in debug mode this will get overwritten + // with a more specific error message to help narrow down things. + $error['detail'] = 'The requested resource could not be found on the server.'; + } + if (config('app.debug')) { $error = array_merge($error, [ 'detail' => $exception->getMessage(), diff --git a/app/Http/Controllers/Api/Client/Servers/SubuserController.php b/app/Http/Controllers/Api/Client/Servers/SubuserController.php index da6fee428..d8bdcc40a 100644 --- a/app/Http/Controllers/Api/Client/Servers/SubuserController.php +++ b/app/Http/Controllers/Api/Client/Servers/SubuserController.php @@ -3,7 +3,9 @@ namespace Pterodactyl\Http\Controllers\Api\Client\Servers; use Illuminate\Http\Request; +use Pterodactyl\Models\User; use Pterodactyl\Models\Server; +use Pterodactyl\Models\Subuser; use Illuminate\Http\JsonResponse; use Pterodactyl\Models\Permission; use Pterodactyl\Repositories\Eloquent\SubuserRepository; @@ -57,6 +59,21 @@ class SubuserController extends ClientApiController ->toArray(); } + /** + * Returns a single subuser associated with this server instance. + * + * @param \Pterodactyl\Http\Requests\Api\Client\Servers\Subusers\GetSubuserRequest $request + * @return array + */ + public function view(GetSubuserRequest $request) + { + $subuser = $request->attributes->get('subuser'); + + return $this->fractal->item($subuser) + ->transformWith($this->getTransformer(SubuserTransformer::class)) + ->toArray(); + } + /** * Create a new subuser for the given server. * @@ -84,15 +101,16 @@ class SubuserController extends ClientApiController * Update a given subuser in the system for the server. * * @param \Pterodactyl\Http\Requests\Api\Client\Servers\Subusers\UpdateSubuserRequest $request - * @param \Pterodactyl\Models\Server $server * @return array * * @throws \Pterodactyl\Exceptions\Model\DataValidationException * @throws \Pterodactyl\Exceptions\Repository\RecordNotFoundException */ - public function update(UpdateSubuserRequest $request, Server $server): array + public function update(UpdateSubuserRequest $request): array { - $subuser = $request->endpointSubuser(); + /** @var \Pterodactyl\Models\Subuser $subuser */ + $subuser = $request->attributes->get('subuser'); + $this->repository->update($subuser->id, [ 'permissions' => $this->getDefaultPermissions($request), ]); @@ -106,14 +124,16 @@ class SubuserController extends ClientApiController * Removes a subusers from a server's assignment. * * @param \Pterodactyl\Http\Requests\Api\Client\Servers\Subusers\DeleteSubuserRequest $request - * @param \Pterodactyl\Models\Server $server * @return \Illuminate\Http\JsonResponse */ - public function delete(DeleteSubuserRequest $request, Server $server) + public function delete(DeleteSubuserRequest $request) { - $this->repository->delete($request->endpointSubuser()->id); + /** @var \Pterodactyl\Models\Subuser $subuser */ + $subuser = $request->attributes->get('subuser'); - return JsonResponse::create([], JsonResponse::HTTP_NO_CONTENT); + $this->repository->delete($subuser->id); + + return new JsonResponse([], JsonResponse::HTTP_NO_CONTENT); } /** diff --git a/app/Http/Middleware/Api/Client/Server/SubuserBelongsToServer.php b/app/Http/Middleware/Api/Client/Server/SubuserBelongsToServer.php new file mode 100644 index 000000000..894d6b000 --- /dev/null +++ b/app/Http/Middleware/Api/Client/Server/SubuserBelongsToServer.php @@ -0,0 +1,37 @@ +route()->parameter('server'); + /** @var \Pterodactyl\Models\User $user */ + $user = $request->route()->parameter('user'); + + // Don't do anything if there isn't a user present in the request. + if (is_null($user)) { + return $next($request); + } + + $request->attributes->set('subuser', $server->subusers()->where('user_id', $user->id)->firstOrFail()); + + return $next($request); + } +} diff --git a/app/Http/Middleware/Api/Client/SubstituteClientApiBindings.php b/app/Http/Middleware/Api/Client/SubstituteClientApiBindings.php index 0bd40eee5..77879c97f 100644 --- a/app/Http/Middleware/Api/Client/SubstituteClientApiBindings.php +++ b/app/Http/Middleware/Api/Client/SubstituteClientApiBindings.php @@ -3,6 +3,7 @@ namespace Pterodactyl\Http\Middleware\Api\Client; use Closure; +use Pterodactyl\Models\User; use Pterodactyl\Models\Backup; use Pterodactyl\Models\Database; use Illuminate\Container\Container; @@ -52,6 +53,10 @@ class SubstituteClientApiBindings extends ApiSubstituteBindings return Backup::query()->where('uuid', $value)->firstOrFail(); }); + $this->router->model('user', User::class, function ($value) { + return User::query()->where('uuid', $value)->firstOrFail(); + }); + return parent::handle($request, $next); } } diff --git a/app/Http/Requests/Api/Client/Servers/Subusers/SubuserRequest.php b/app/Http/Requests/Api/Client/Servers/Subusers/SubuserRequest.php index e43b7178e..98d0d9643 100644 --- a/app/Http/Requests/Api/Client/Servers/Subusers/SubuserRequest.php +++ b/app/Http/Requests/Api/Client/Servers/Subusers/SubuserRequest.php @@ -3,12 +3,10 @@ namespace Pterodactyl\Http\Requests\Api\Client\Servers\Subusers; use Illuminate\Http\Request; -use Pterodactyl\Models\Server; +use Pterodactyl\Models\User; use Pterodactyl\Exceptions\Http\HttpForbiddenException; -use Pterodactyl\Repositories\Eloquent\SubuserRepository; use Pterodactyl\Http\Requests\Api\Client\ClientApiRequest; -use Pterodactyl\Exceptions\Repository\RecordNotFoundException; -use Symfony\Component\HttpKernel\Exception\NotFoundHttpException; +use Pterodactyl\Services\Servers\GetUserPermissionsService; abstract class SubuserRequest extends ClientApiRequest { @@ -30,10 +28,10 @@ abstract class SubuserRequest extends ClientApiRequest return false; } - // If there is a subuser present in the URL, validate that it is not the same as the - // current request user. You're not allowed to modify yourself. - if ($this->route()->hasParameter('subuser')) { - if ($this->endpointSubuser()->user_id === $this->user()->id) { + $user = $this->route()->parameter('user'); + // Don't allow a user to edit themselves on the server. + if ($user instanceof User) { + if ($user->uuid === $this->user()->uuid) { return false; } } @@ -71,68 +69,14 @@ abstract class SubuserRequest extends ClientApiRequest // Otherwise, get the current subuser's permission set, and ensure that the // permissions they are trying to assign are not _more_ than the ones they // already have. - if (count(array_diff($permissions, $this->currentUserPermissions())) > 0) { + /** @var \Pterodactyl\Models\Subuser|null $subuser */ + /** @var \Pterodactyl\Services\Servers\GetUserPermissionsService $service */ + $service = $this->container->make(GetUserPermissionsService::class); + + if (count(array_diff($permissions, $service->handle($server, $user))) > 0) { throw new HttpForbiddenException( 'Cannot assign permissions to a subuser that your account does not actively possess.' ); } } - - /** - * Returns the currently authenticated user's permissions. - * - * @return array - * - * @throws \Illuminate\Contracts\Container\BindingResolutionException - */ - public function currentUserPermissions(): array - { - /** @var \Pterodactyl\Repositories\Eloquent\SubuserRepository $repository */ - $repository = $this->container->make(SubuserRepository::class); - - /* @var \Pterodactyl\Models\Subuser $model */ - try { - $model = $repository->findFirstWhere([ - ['server_id', $this->route()->parameter('server')->id], - ['user_id', $this->user()->id], - ]); - } catch (RecordNotFoundException $exception) { - return []; - } - - return $model->permissions; - } - - /** - * Return the subuser model for the given request which can then be validated. If - * required request parameters are missing a 404 error will be returned, otherwise - * a model exception will be returned if the model is not found. - * - * This returns the subuser based on the endpoint being hit, not the actual subuser - * for the account making the request. - * - * @return \Pterodactyl\Models\Subuser - * - * @throws \Symfony\Component\HttpKernel\Exception\NotFoundHttpException - * @throws \Illuminate\Database\Eloquent\ModelNotFoundException - * @throws \Illuminate\Contracts\Container\BindingResolutionException - */ - public function endpointSubuser() - { - /** @var \Pterodactyl\Repositories\Eloquent\SubuserRepository $repository */ - $repository = $this->container->make(SubuserRepository::class); - - $parameters = $this->route()->parameters(); - if ( - ! isset($parameters['server'], $parameters['server']) - || ! is_string($parameters['subuser']) - || ! $parameters['server'] instanceof Server - ) { - throw new NotFoundHttpException; - } - - return $this->model ?: $this->model = $repository->getUserForServer( - $parameters['server']->id, $parameters['subuser'] - ); - } } diff --git a/app/Models/Server.php b/app/Models/Server.php index 8f15bfcf1..8894a4d60 100644 --- a/app/Models/Server.php +++ b/app/Models/Server.php @@ -38,7 +38,7 @@ use Znck\Eloquent\Traits\BelongsToThrough; * @property \Carbon\Carbon $updated_at * * @property \Pterodactyl\Models\User $user - * @property \Pterodactyl\Models\User[]|\Illuminate\Database\Eloquent\Collection $subusers + * @property \Pterodactyl\Models\Subuser[]|\Illuminate\Database\Eloquent\Collection $subusers * @property \Pterodactyl\Models\Allocation $allocation * @property \Pterodactyl\Models\Allocation[]|\Illuminate\Database\Eloquent\Collection $allocations * @property \Pterodactyl\Models\Pack|null $pack diff --git a/app/Repositories/Eloquent/SubuserRepository.php b/app/Repositories/Eloquent/SubuserRepository.php index e00d825e7..c0fb930a6 100644 --- a/app/Repositories/Eloquent/SubuserRepository.php +++ b/app/Repositories/Eloquent/SubuserRepository.php @@ -18,30 +18,6 @@ class SubuserRepository extends EloquentRepository implements SubuserRepositoryI return Subuser::class; } - /** - * Returns a subuser model for the given user and server combination. If no record - * exists an exception will be thrown. - * - * @param int $server - * @param string $uuid - * @return \Pterodactyl\Models\Subuser - * - * @throws \Illuminate\Database\Eloquent\ModelNotFoundException - */ - public function getUserForServer(int $server, string $uuid): Subuser - { - /** @var \Pterodactyl\Models\Subuser $model */ - $model = $this->getBuilder() - ->with('server', 'user') - ->select('subusers.*') - ->join('users', 'users.id', '=', 'subusers.user_id') - ->where('subusers.server_id', $server) - ->where('users.uuid', $uuid) - ->firstOrFail(); - - return $model; - } - /** * Return a subuser with the associated server relationship. * diff --git a/app/Services/Servers/GetUserPermissionsService.php b/app/Services/Servers/GetUserPermissionsService.php index 98dcf6c34..e0ea20373 100644 --- a/app/Services/Servers/GetUserPermissionsService.php +++ b/app/Services/Servers/GetUserPermissionsService.php @@ -30,7 +30,7 @@ class GetUserPermissionsService } /** @var \Pterodactyl\Models\Subuser|null $subuserPermissions */ - $subuserPermissions = $server->subusers->where('user_id', $user->id)->first(); + $subuserPermissions = $server->subusers()->where('user_id', $user->id)->first(); return $subuserPermissions ? $subuserPermissions->permissions : []; } diff --git a/routes/api-client.php b/routes/api-client.php index f92ba6ed9..c9ec16097 100644 --- a/routes/api-client.php +++ b/routes/api-client.php @@ -1,6 +1,7 @@ '/servers/{server}', 'middleware' => [AuthenticateServ Route::delete('/allocations/{allocation}', 'Servers\NetworkAllocationController@delete'); }); - Route::group(['prefix' => '/users'], function () { + Route::group(['prefix' => '/users', 'middleware' => [SubuserBelongsToServer::class]], function () { Route::get('/', 'Servers\SubuserController@index'); Route::post('/', 'Servers\SubuserController@store'); - Route::get('/{subuser}', 'Servers\SubuserController@view'); - Route::post('/{subuser}', 'Servers\SubuserController@update'); - Route::delete('/{subuser}', 'Servers\SubuserController@delete'); + Route::get('/{user}', 'Servers\SubuserController@view'); + Route::post('/{user}', 'Servers\SubuserController@update'); + Route::delete('/{user}', 'Servers\SubuserController@delete'); }); Route::group(['prefix' => '/backups'], function () {