From c748fa984272fb519384bd7fc522f10e05d3f04f Mon Sep 17 00:00:00 2001 From: Dane Everitt Date: Sun, 9 Oct 2022 15:14:16 -0700 Subject: [PATCH] fix: exclude any permissions not defined internally when updating or creating subusers (#4416) --- .../Api/Client/Servers/SubuserController.php | 17 ++- app/Policies/ServerPolicy.php | 35 +---- .../Server/Subuser/UpdateSubuserTest.php | 133 ++++++++++++++++++ 3 files changed, 154 insertions(+), 31 deletions(-) create mode 100644 tests/Integration/Api/Client/Server/Subuser/UpdateSubuserTest.php diff --git a/app/Http/Controllers/Api/Client/Servers/SubuserController.php b/app/Http/Controllers/Api/Client/Servers/SubuserController.php index ab7e5003a..dfa503e48 100644 --- a/app/Http/Controllers/Api/Client/Servers/SubuserController.php +++ b/app/Http/Controllers/Api/Client/Servers/SubuserController.php @@ -190,10 +190,23 @@ class SubuserController extends ClientApiController } /** - * Returns the default permissions for all subusers to ensure none are ever removed wrongly. + * Returns the default permissions for subusers and parses out any permissions + * that were passed that do not also exist in the internally tracked list of + * permissions. */ protected function getDefaultPermissions(Request $request): array { - return array_unique(array_merge($request->input('permissions') ?? [], [Permission::ACTION_WEBSOCKET_CONNECT])); + $allowed = Permission::permissions() + ->map(function ($value, $prefix) { + return array_map(function ($value) use ($prefix) { + return "$prefix.$value"; + }, array_keys($value['keys'])); + }) + ->flatten() + ->all(); + + $cleaned = array_intersect($request->input('permissions') ?? [], $allowed); + + return array_unique(array_merge($cleaned, [Permission::ACTION_WEBSOCKET_CONNECT])); } } diff --git a/app/Policies/ServerPolicy.php b/app/Policies/ServerPolicy.php index c821bb42f..d72b7bd14 100644 --- a/app/Policies/ServerPolicy.php +++ b/app/Policies/ServerPolicy.php @@ -2,45 +2,22 @@ namespace Pterodactyl\Policies; -use Carbon\Carbon; use Pterodactyl\Models\User; use Pterodactyl\Models\Server; -use Illuminate\Contracts\Cache\Repository as CacheRepository; class ServerPolicy { - /** - * @var \Illuminate\Contracts\Cache\Repository - */ - private $cache; - - /** - * ServerPolicy constructor. - */ - public function __construct(CacheRepository $cache) - { - $this->cache = $cache; - } - /** * Checks if the user has the given permission on/for the server. - * - * @param string $permission - * - * @return bool */ - protected function checkPermission(User $user, Server $server, $permission) + protected function checkPermission(User $user, Server $server, string $permission): bool { - $key = sprintf('ServerPolicy.%s.%s', $user->uuid, $server->uuid); + $subuser = $server->subusers->where('user_id', $user->id)->first(); + if (!$subuser || empty($permission)) { + return false; + } - $permissions = $this->cache->remember($key, Carbon::now()->addSeconds(5), function () use ($user, $server) { - /** @var \Pterodactyl\Models\Subuser|null $subuser */ - $subuser = $server->subusers()->where('user_id', $user->id)->first(); - - return $subuser ? $subuser->permissions : []; - }); - - return in_array($permission, $permissions); + return in_array($permission, $subuser->permissions); } /** diff --git a/tests/Integration/Api/Client/Server/Subuser/UpdateSubuserTest.php b/tests/Integration/Api/Client/Server/Subuser/UpdateSubuserTest.php new file mode 100644 index 000000000..5990b9618 --- /dev/null +++ b/tests/Integration/Api/Client/Server/Subuser/UpdateSubuserTest.php @@ -0,0 +1,133 @@ +generateTestAccount(['user.read']); + + $subuser = Subuser::factory() + ->for(User::factory()->create()) + ->for($server) + ->create([ + 'permissions' => ['control.start'], + ]); + + $this->postJson( + $endpoint = "/api/client/servers/$server->uuid/users/{$subuser->user->uuid}", + $data = [ + 'permissions' => [ + 'control.start', + 'control.stop', + ], + ] + ) + ->assertUnauthorized(); + + $this->actingAs($subuser->user)->postJson($endpoint, $data)->assertForbidden(); + $this->actingAs($user)->postJson($endpoint, $data)->assertForbidden(); + + $server->subusers()->where('user_id', $user->id)->update([ + 'permissions' => [ + Permission::ACTION_USER_UPDATE, + Permission::ACTION_CONTROL_START, + Permission::ACTION_CONTROL_STOP, + ], + ]); + + $this->postJson($endpoint, $data)->assertOk(); + } + + /** + * Tests that permissions for the account are updated and any extraneous values + * we don't know about are removed. + */ + public function testPermissionsAreSavedToAccount() + { + [$user, $server] = $this->generateTestAccount(); + + /** @var \Pterodactyl\Models\Subuser $subuser */ + $subuser = Subuser::factory() + ->for(User::factory()->create()) + ->for($server) + ->create([ + 'permissions' => ['control.restart', 'websocket.connect', 'foo.bar'], + ]); + + $this->actingAs($user) + ->postJson("/api/client/servers/$server->uuid/users/{$subuser->user->uuid}", [ + 'permissions' => [ + 'control.start', + 'control.stop', + 'control.stop', + 'foo.bar', + 'power.fake', + ], + ]) + ->assertOk(); + + $subuser->refresh(); + $this->assertEqualsCanonicalizing( + ['control.start', 'control.stop', 'websocket.connect'], + $subuser->permissions + ); + } + + /** + * Ensure a subuser cannot assign permissions to an account that they do not have + * themselves. + */ + public function testUserCannotAssignPermissionsTheyDoNotHave() + { + [$user, $server] = $this->generateTestAccount([Permission::ACTION_USER_READ, Permission::ACTION_USER_UPDATE]); + + $subuser = Subuser::factory() + ->for(User::factory()->create()) + ->for($server) + ->create(['permissions' => ['foo.bar']]); + + $this->actingAs($user) + ->postJson("/api/client/servers/$server->uuid/users/{$subuser->user->uuid}", [ + 'permissions' => [Permission::ACTION_USER_READ, Permission::ACTION_CONTROL_CONSOLE], + ]) + ->assertForbidden(); + + $this->assertEqualsCanonicalizing(['foo.bar'], $subuser->refresh()->permissions); + } + + /** + * Test that a user cannot update thyself. + */ + public function testUserCannotUpdateSelf() + { + [$user, $server] = $this->generateTestAccount([Permission::ACTION_USER_READ, Permission::ACTION_USER_UPDATE]); + + $this->actingAs($user) + ->postJson("/api/client/servers/$server->uuid/users/$user->uuid", []) + ->assertForbidden(); + } + + /** + * Test that an error is returned if you attempt to update a subuser on a different account. + */ + public function testCannotUpdateSubuserForDifferentServer() + { + [$user, $server] = $this->generateTestAccount(); + [$user2] = $this->generateTestAccount(['foo.bar']); + + $this->actingAs($user) + ->postJson("/api/client/servers/$server->uuid/users/$user2->uuid", []) + ->assertNotFound(); + } +}