fix: exclude any permissions not defined internally when updating or creating subusers (#4416)

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]));
     }
 }

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) {
+        return in_array($permission, $subuser->permissions);
-            /** @var \Pterodactyl\Models\Subuser|null $subuser */
-            $subuser = $server->subusers()->where('user_id', $user->id)->first();
-
-            return $subuser ? $subuser->permissions : [];
-        });
-
-        return in_array($permission, $permissions);
     }
 
     /**

tests/Integration/Api/Client/Server/Subuser/UpdateSubuserTest.php

@@ -0,0 +1,133 @@
+<?php
+
+namespace Pterodactyl\Tests\Integration\Api\Client\Server\Subuser;
+
+use Pterodactyl\Models\User;
+use Pterodactyl\Models\Subuser;
+use Pterodactyl\Models\Permission;
+use Pterodactyl\Tests\Integration\Api\Client\ClientApiIntegrationTestCase;
+
+class UpdateSubuserTest extends ClientApiIntegrationTestCase
+{
+    /**
+     * Test that the correct permissions are applied to the account when making updates
+     * to a subusers permissions.
+     */
+    public function testCorrectPermissionsAreRequiredForUpdating()
+    {
+        [$user, $server] = $this->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();
+    }
+}