Fix server access for admins without subuser (#919)

* fix server access for admins without subuser * add permission checks to power buttons * add permission check for console command sending * fix tests * fix websocket token permissions * fix sftp access * fix server api + small cleanup * it's "update", not "edit"... * fix tests * fix permission const for "activity read" * fix activity subuser permission
pelican-dev · Jan 17, 2025 · 03eaddb · 03eaddb
1 parent 61bdf0d
commit 03eaddb
Show file tree

Hide file tree

Showing 13 changed files with 103 additions and 45 deletions.
diff --git a/app/Filament/App/Resources/ServerResource/Pages/ListServers.php b/app/Filament/App/Resources/ServerResource/Pages/ListServers.php
@@ -19,7 +19,7 @@ class ListServers extends ListRecords
 
     public function table(Table $table): Table
     {
-        $baseQuery = auth()->user()->can('viewList server') ? Server::query() : auth()->user()->accessibleServers();
+        $baseQuery = auth()->user()->accessibleServers();
 
         return $table
             ->paginated(false)

diff --git a/app/Filament/Server/Pages/Console.php b/app/Filament/Server/Pages/Console.php
@@ -10,6 +10,7 @@
 // use App\Filament\Server\Widgets\ServerNetworkChart;
 use App\Filament\Server\Widgets\ServerOverview;
 use App\Livewire\AlertBanner;
+use App\Models\Permission;
 use App\Models\Server;
 use Filament\Actions\Action;
 use Filament\Facades\Filament;
@@ -94,16 +95,19 @@ protected function getHeaderActions(): array
                 ->color('primary')
                 ->size(ActionSize::ExtraLarge)
                 ->action(fn () => $this->dispatch('setServerState', state: 'start', uuid: $server->uuid))
+                ->authorize(fn () => auth()->user()->can(Permission::ACTION_CONTROL_START, $server))
                 ->disabled(fn () => $server->isInConflictState() || !$this->status->isStartable()),
             Action::make('restart')
                 ->color('gray')
                 ->size(ActionSize::ExtraLarge)
                 ->action(fn () => $this->dispatch('setServerState', state: 'restart', uuid: $server->uuid))
+                ->authorize(fn () => auth()->user()->can(Permission::ACTION_CONTROL_RESTART, $server))
                 ->disabled(fn () => $server->isInConflictState() || !$this->status->isRestartable()),
             Action::make('stop')
                 ->color('danger')
                 ->size(ActionSize::ExtraLarge)
                 ->action(fn () => $this->dispatch('setServerState', state: 'stop', uuid: $server->uuid))
+                ->authorize(fn () => auth()->user()->can(Permission::ACTION_CONTROL_STOP, $server))
                 ->hidden(fn () => $this->status->isStartingOrStopping() || $this->status->isKillable())
                 ->disabled(fn () => $server->isInConflictState() || !$this->status->isStoppable()),
             Action::make('kill')
@@ -114,6 +118,7 @@ protected function getHeaderActions(): array
                 ->modalSubmitActionLabel('Kill Server')
                 ->size(ActionSize::ExtraLarge)
                 ->action(fn () => $this->dispatch('setServerState', state: 'kill', uuid: $server->uuid))
+                ->authorize(fn () => auth()->user()->can(Permission::ACTION_CONTROL_STOP, $server))
                 ->hidden(fn () => $server->isInConflictState() || !$this->status->isKillable()),
         ];
     }

diff --git a/app/Filament/Server/Resources/UserResource/Pages/ListUsers.php b/app/Filament/Server/Resources/UserResource/Pages/ListUsers.php
@@ -115,7 +115,9 @@ protected function getHeaderActions(): array
                                             'settings' => [
                                                 'rename',
                                                 'reinstall',
-                                                'activity',
+                                            ],
+                                            'activity' => [
+                                                'read',
                                             ],
                                         ];
 
@@ -357,6 +359,24 @@ protected function getHeaderActions(): array
                                                         ]),
                                                 ]),
                                         ]),
+                                    Tabs\Tab::make('Activity')
+                                        ->schema([
+                                            Section::make()
+                                                ->description(trans('server/users.permissions.activity_desc'))
+                                                ->icon('tabler-stack')
+                                                ->schema([
+                                                    CheckboxList::make('activity')
+                                                        ->bulkToggleable()
+                                                        ->label('')
+                                                        ->columns(2)
+                                                        ->options([
+                                                            'read' => 'Read',
+                                                        ])
+                                                        ->descriptions([
+                                                            'read' => trans('server/users.permissions.activity_read'),
+                                                        ]),
+                                                ]),
+                                        ]),
                                 ]),
 
                         ]),

diff --git a/app/Filament/Server/Widgets/ServerConsole.php b/app/Filament/Server/Widgets/ServerConsole.php
@@ -67,9 +67,14 @@ protected function getSocket(): string
         return $socket;
     }
 
+    protected function authorizeSendCommand(): bool
+    {
+        return $this->user->can(Permission::ACTION_CONTROL_CONSOLE, $this->server);
+    }
+
     protected function canSendCommand(): bool
     {
-        return !$this->server->isInConflictState() && $this->server->retrieveStatus() === 'running';
+        return $this->authorizeSendCommand() && !$this->server->isInConflictState() && $this->server->retrieveStatus() === 'running';
     }
 
     public function up(): void

diff --git a/app/Http/Controllers/Api/Client/ClientController.php b/app/Http/Controllers/Api/Client/ClientController.php
@@ -53,12 +53,12 @@ public function index(GetServersRequest $request): array
             } else {
                 $builder = $type === 'admin-all'
                     ? $builder
-                    : $builder->whereNotIn('servers.id', $user->accessibleServers()->pluck('id')->all());
+                    : $builder->whereNotIn('servers.id', $user->directAccessibleServers()->pluck('id')->all());
             }
         } elseif ($type === 'owner') {
             $builder = $builder->where('servers.owner_id', $user->id);
         } else {
-            $builder = $builder->whereIn('servers.id', $user->accessibleServers()->pluck('id')->all());
+            $builder = $builder->whereIn('servers.id', $user->directAccessibleServers()->pluck('id')->all());
         }
 
         $servers = $builder->paginate(min($request->query('per_page', '50'), 100))->appends($request->query());

diff --git a/app/Http/Controllers/Api/Remote/SftpAuthenticationController.php b/app/Http/Controllers/Api/Remote/SftpAuthenticationController.php
@@ -138,7 +138,7 @@ protected function reject(Request $request, bool $increment = true): void
      */
     protected function validateSftpAccess(User $user, Server $server): void
     {
-        if (!$user->isRootAdmin() && $server->owner_id !== $user->id) {
+        if ($user->cannot('update server', $server) && $server->owner_id !== $user->id) {
             $permissions = $this->permissions->handle($server, $user);
 
             if (!in_array(Permission::ACTION_FILE_SFTP, $permissions)) {

diff --git a/app/Http/Middleware/Api/Client/Server/AuthenticateServerAccess.php b/app/Http/Middleware/Api/Client/Server/AuthenticateServerAccess.php
@@ -35,9 +35,9 @@ public function handle(Request $request, \Closure $next): mixed
         }
 
         // At the very least, ensure that the user trying to make this request is the
-        // server owner, a subuser, or a root admin. We'll leave it up to the controllers
+        // server owner, a subuser, or an admin. We'll leave it up to the controllers
         // to authenticate more detailed permissions if needed.
-        if ($user->id !== $server->owner_id && !$user->isRootAdmin()) {
+        if ($user->id !== $server->owner_id && $user->cannot('update server', $server)) {
             // Check for subuser status.
             if (!$server->subusers->contains('user_id', $user->id)) {
                 throw new NotFoundHttpException(trans('exceptions.api.resource_not_found'));
@@ -53,7 +53,7 @@ public function handle(Request $request, \Closure $next): mixed
                 if (($server->isSuspended() || $server->node->isUnderMaintenance()) && !$request->routeIs('api:client:server.resources')) {
                     throw $exception;
                 }
-                if (!$user->isRootAdmin() || !$request->routeIs($this->except)) {
+                if ($user->cannot('update server', $server) || !$request->routeIs($this->except)) {
                     throw $exception;
                 }
             }

diff --git a/app/Http/Requests/Api/Client/Servers/Subusers/SubuserRequest.php b/app/Http/Requests/Api/Client/Servers/Subusers/SubuserRequest.php
@@ -55,8 +55,8 @@ protected function validatePermissionsCanBeAssigned(array $permissions): void
         /** @var \App\Models\Server $server */
         $server = $this->route()->parameter('server');
 
-        // If we are a root admin or the server owner, no need to perform these checks.
-        if ($user->isRootAdmin() || $user->id === $server->owner_id) {
+        // If we are an admin or the server owner, no need to perform these checks.
+        if ($user->can('update server', $server) || $user->id === $server->owner_id) {
             return;
         }
 

diff --git a/app/Models/Permission.php b/app/Models/Permission.php
@@ -93,7 +93,7 @@ class Permission extends Model
 
     public const ACTION_SETTINGS_REINSTALL = 'settings.reinstall';
 
-    public const ACTION_ACTIVITY_READ = 'settings.activity';
+    public const ACTION_ACTIVITY_READ = 'activity.read';
 
     /**
      * Should timestamps be used on this model.

diff --git a/app/Models/User.php b/app/Models/User.php
@@ -289,10 +289,23 @@ public function activity(): MorphToMany
     }
 
     /**
-     * Returns all the servers that a user can access by way of being the owner of the
-     * server, or because they are assigned as a subuser for that server.
+     * Returns all the servers that a user can access.
+     * Either because they are an admin or because they are the owner/ a subuser of the server.
      */
     public function accessibleServers(): Builder
+    {
+        if ($this->canned('viewList server')) {
+            return Server::query();
+        }
+
+        return $this->directAccessibleServers();
+    }
+
+    /**
+     * Returns all the servers that a user can access "directly".
+     * This means either because they are the owner or a subuser of the server.
+     */
+    public function directAccessibleServers(): Builder
     {
         return Server::query()
             ->select('servers.*')
@@ -315,7 +328,12 @@ public function subServers(): BelongsToMany
 
     protected function checkPermission(Server $server, string $permission = ''): bool
     {
-        if ($this->isRootAdmin() || $server->owner_id === $this->id) {
+        if ($this->canned('update server', $server) || $server->owner_id === $this->id) {
+            return true;
+        }
+
+        // If the user only has "view" permissions allow viewing the console
+        if ($permission === Permission::ACTION_WEBSOCKET_CONNECT && $this->canned('view server', $server)) {
             return true;
         }
 
@@ -361,14 +379,19 @@ public function isRootAdmin(): bool
         return $this->hasRole(Role::ROOT_ADMIN);
     }
 
+    public function isAdmin(): bool
+    {
+        return $this->isRootAdmin() || ($this->roles()->count() >= 1 && $this->getAllPermissions()->count() >= 1);
+    }
+
     public function canAccessPanel(Panel $panel): bool
     {
         if ($this->isRootAdmin()) {
             return true;
         }
 
         if ($panel->getId() === 'admin') {
-            return $this->roles()->count() >= 1 && $this->getAllPermissions()->count() >= 1;
+            return $this->isAdmin();
         }
 
         return true;
@@ -401,7 +424,7 @@ public function getTenants(Panel $panel): array|Collection
     public function canAccessTenant(IlluminateModel $tenant): bool
     {
         if ($tenant instanceof Server) {
-            if ($this->isRootAdmin() || $tenant->owner_id === $this->id) {
+            if ($this->canned('view server', $tenant) || $tenant->owner_id === $this->id) {
                 return true;
             }
 

diff --git a/app/Services/Servers/GetUserPermissionsService.php b/app/Services/Servers/GetUserPermissionsService.php
@@ -9,23 +9,25 @@ class GetUserPermissionsService
 {
     /**
      * Returns the server specific permissions that a user has. This checks
-     * if they are an admin or a subuser for the server. If no permissions are
-     * found, an empty array is returned.
+     * if they are an admin, the owner or a subuser for the server. If no
+     * permissions are found, an empty array is returned.
      */
     public function handle(Server $server, User $user): array
     {
-        if ($user->isRootAdmin() || $user->id === $server->owner_id) {
-            $permissions = ['*'];
+        if ($user->isAdmin() && ($user->can('view server', $server) || $user->can('update server', $server))) {
+            $permissions = $user->can('update server', $server) ? ['*'] : ['websocket.connect', 'backup.read'];
 
-            if ($user->isRootAdmin()) {
-                $permissions[] = 'admin.websocket.errors';
-                $permissions[] = 'admin.websocket.install';
-                $permissions[] = 'admin.websocket.transfer';
-            }
+            $permissions[] = 'admin.websocket.errors';
+            $permissions[] = 'admin.websocket.install';
+            $permissions[] = 'admin.websocket.transfer';
 
             return $permissions;
         }
 
+        if ($user->id === $server->owner_id) {
+            return ['*'];
+        }
+
         /** @var \App\Models\Subuser|null $subuserPermissions */
         $subuserPermissions = $server->subusers()->where('user_id', $user->id)->first();
 

diff --git a/lang/en/server/users.php b/lang/en/server/users.php
@@ -2,8 +2,9 @@
 
 return [
     'permissions' => [
+        'activity_desc' => 'Permissions that control a user\'s access to the server activity logs.',
         'startup_desc' => 'Permissions that control a user\'s ability to view this server\'s startup parameters.',
-        'settings_desc' => 'Permissions that control a user\'s access to the schedule management for this server.',
+        'settings_desc' => 'Permissions that control a user\'s ability to modify this server\'s settings.',
         'control_desc' => 'Permissions that control a user\'s ability to control the power state of a server, or send commands.',
         'user_desc' => 'Permissions that allow a user to manage other subusers on a server. They will never be able to edit their own account, or assign permissions they do not have themselves.',
         'file_desc' => 'Permissions that control a user\'s ability to modify the filesystem for this server.',
@@ -16,7 +17,7 @@
         'startup_docker_image' => 'Allows a user to modify the Docker image used when running the server.',
         'setting_reinstall' => 'Allows a user to trigger a reinstall of this server.',
         'setting_rename' => 'Allows a user to rename this server and change the description of it.',
-        'setting_activity' => 'Allows a user to view the activity logs for the server.',
+        'activity_read' => 'Allows a user to view the activity logs for the server.',
         'websocket_*' => 'Allows a user access to the websocket for this server.',
         'control_console' => 'Allows a user to send data to the server console.',
         'control_start' => 'Allows a user to start the server instance.',

diff --git a/resources/views/filament/components/server-console.blade.php b/resources/views/filament/components/server-console.blade.php
@@ -11,23 +11,25 @@
 
     <div id="terminal" wire:ignore></div>
 
-    <div class="flex items-center w-full border-top overflow-hidden dark:bg-gray-900"
-         style="border-bottom-right-radius: 10px; border-bottom-left-radius: 10px;">
-        <x-filament::icon
-            icon="tabler-chevrons-right"
-        />
-        <input
-            class="w-full focus:outline-none focus:ring-0 border-none dark:bg-gray-900"
-            type="text"
-            :readonly="{{ $this->canSendCommand() ? 'false' : 'true' }}"
-            title="{{ $this->canSendCommand() ? '' : 'Can\'t send command when the server is Offline' }}"
-            placeholder="{{ $this->canSendCommand() ? 'Type a command...' : 'Server Offline...' }}"
-            wire:model="input"
-            wire:keydown.enter="enter"
-            wire:keydown.up.prevent="up"
-            wire:keydown.down="down"
-        >
-    </div>
+    @if ($this->authorizeSendCommand())
+        <div class="flex items-center w-full border-top overflow-hidden dark:bg-gray-900"
+            style="border-bottom-right-radius: 10px; border-bottom-left-radius: 10px;">
+            <x-filament::icon
+                icon="tabler-chevrons-right"
+            />
+            <input
+                class="w-full focus:outline-none focus:ring-0 border-none dark:bg-gray-900"
+                type="text"
+                :readonly="{{ $this->canSendCommand() ? 'false' : 'true' }}"
+                title="{{ $this->canSendCommand() ? '' : 'Can\'t send command when the server is Offline' }}"
+                placeholder="{{ $this->canSendCommand() ? 'Type a command...' : 'Server Offline...' }}"
+                wire:model="input"
+                wire:keydown.enter="enter"
+                wire:keydown.up.prevent="up"
+                wire:keydown.down="down"
+            >
+        </div>
+    @endif
 
     @script
     <script>