From 35ce1d34aba0c7908c60ed638b2e6fc9a4f356d0 Mon Sep 17 00:00:00 2001
From: Boy132 <Boy132@users.noreply.github.com>
Date: Tue, 27 May 2025 19:30:30 +0200
Subject: [PATCH] Permission check fixes (#1406)

---
 .../Admin/Resources/ApiKeyResource.php         |  2 +-
 .../DatabasesRelationManager.php               |  4 ++--
 .../AllocationsRelationManager.php             |  2 +-
 .../Admin/Resources/ServerResource.php         |  4 +---
 .../ServerResource/Pages/CreateServer.php      |  2 ++
 .../ServerResource/Pages/EditServer.php        | 10 +++++-----
 .../ServerResource/Pages/ListServers.php       |  4 ++--
 .../Actions/RotateDatabasePasswordAction.php   |  2 +-
 .../Server/Resources/ActivityResource.php      |  4 ++--
 .../Client/Servers/Subusers/SubuserRequest.php |  2 +-
 app/Models/User.php                            | 18 +++++++++++-------
 app/Policies/ServerPolicy.php                  | 10 +++++-----
 .../Servers/GetUserPermissionsService.php      |  4 ++--
 13 files changed, 36 insertions(+), 32 deletions(-)

diff --git a/app/Filament/Admin/Resources/ApiKeyResource.php b/app/Filament/Admin/Resources/ApiKeyResource.php
index c02af4f54..2e6646e48 100644
--- a/app/Filament/Admin/Resources/ApiKeyResource.php
+++ b/app/Filament/Admin/Resources/ApiKeyResource.php
@@ -79,7 +79,7 @@ class ApiKeyResource extends Resource
                 TextColumn::make('user.username')
                     ->label(trans('admin/apikey.table.created_by'))
                     ->icon('tabler-user')
-                    ->url(fn (ApiKey $apiKey) => auth()->user()->can('update user', $apiKey->user) ? EditUser::getUrl(['record' => $apiKey->user]) : null),
+                    ->url(fn (ApiKey $apiKey) => auth()->user()->can('update', $apiKey->user) ? EditUser::getUrl(['record' => $apiKey->user]) : null),
             ])
             ->actions([
                 DeleteAction::make(),
diff --git a/app/Filament/Admin/Resources/DatabaseHostResource/RelationManagers/DatabasesRelationManager.php b/app/Filament/Admin/Resources/DatabaseHostResource/RelationManagers/DatabasesRelationManager.php
index c8472bad6..da51a140d 100644
--- a/app/Filament/Admin/Resources/DatabaseHostResource/RelationManagers/DatabasesRelationManager.php
+++ b/app/Filament/Admin/Resources/DatabaseHostResource/RelationManagers/DatabasesRelationManager.php
@@ -71,10 +71,10 @@ class DatabasesRelationManager extends RelationManager
             ])
             ->actions([
                 DeleteAction::make()
-                    ->authorize(fn (Database $database) => auth()->user()->can('delete database', $database)),
+                    ->authorize(fn (Database $database) => auth()->user()->can('delete', $database)),
                 ViewAction::make()
                     ->color('primary')
-                    ->hidden(fn () => !auth()->user()->can('viewList database')),
+                    ->hidden(fn () => !auth()->user()->can('viewAny', Database::class)),
             ]);
     }
 }
diff --git a/app/Filament/Admin/Resources/NodeResource/RelationManagers/AllocationsRelationManager.php b/app/Filament/Admin/Resources/NodeResource/RelationManagers/AllocationsRelationManager.php
index 4c11002d0..10a3566ca 100644
--- a/app/Filament/Admin/Resources/NodeResource/RelationManagers/AllocationsRelationManager.php
+++ b/app/Filament/Admin/Resources/NodeResource/RelationManagers/AllocationsRelationManager.php
@@ -97,7 +97,7 @@ class AllocationsRelationManager extends RelationManager
             ])
             ->groupedBulkActions([
                 DeleteBulkAction::make()
-                    ->authorize(fn () => auth()->user()->can('update node')),
+                    ->authorize(fn () => auth()->user()->can('update', $this->getOwnerRecord())),
             ]);
     }
 }
diff --git a/app/Filament/Admin/Resources/ServerResource.php b/app/Filament/Admin/Resources/ServerResource.php
index ec39ab7f8..f8d0e4d6a 100644
--- a/app/Filament/Admin/Resources/ServerResource.php
+++ b/app/Filament/Admin/Resources/ServerResource.php
@@ -79,8 +79,6 @@ class ServerResource extends Resource
     {
         $query = parent::getEloquentQuery();
 
-        return $query->whereHas('node', function (Builder $query) {
-            $query->whereIn('id', auth()->user()->accessibleNodes()->pluck('id'));
-        });
+        return $query->whereIn('node_id', auth()->user()->accessibleNodes()->pluck('id'));
     }
 }
diff --git a/app/Filament/Admin/Resources/ServerResource/Pages/CreateServer.php b/app/Filament/Admin/Resources/ServerResource/Pages/CreateServer.php
index a58a42e48..d49a0140e 100644
--- a/app/Filament/Admin/Resources/ServerResource/Pages/CreateServer.php
+++ b/app/Filament/Admin/Resources/ServerResource/Pages/CreateServer.php
@@ -144,6 +144,7 @@ class CreateServer extends CreateRecord
                                 ->relationship('user', 'username')
                                 ->searchable(['username', 'email'])
                                 ->getOptionLabelFromRecordUsing(fn (User $user) => "$user->username ($user->email)")
+                                ->createOptionAction(fn (Action $action) => $action->authorize(fn () => auth()->user()->can('create', User::class)))
                                 ->createOptionForm([
                                     TextInput::make('username')
                                         ->label(trans('admin/user.username'))
@@ -205,6 +206,7 @@ class CreateServer extends CreateRecord
                                         ->where('node_id', $get('node_id'))
                                         ->whereNull('server_id'),
                                 )
+                                ->createOptionAction(fn (Action $action) => $action->authorize(fn (Get $get) => auth()->user()->can('create', Node::find($get('node_id')))))
                                 ->createOptionForm(function (Get $get) {
                                     $getPage = $get;
 
diff --git a/app/Filament/Admin/Resources/ServerResource/Pages/EditServer.php b/app/Filament/Admin/Resources/ServerResource/Pages/EditServer.php
index d1bb332b3..ecd788df5 100644
--- a/app/Filament/Admin/Resources/ServerResource/Pages/EditServer.php
+++ b/app/Filament/Admin/Resources/ServerResource/Pages/EditServer.php
@@ -686,7 +686,7 @@ class EditServer extends EditRecord
                                 ServerResource::getMountCheckboxList($get),
                             ]),
                         Tab::make(trans('admin/server.databases'))
-                            ->hidden(fn () => !auth()->user()->can('viewList database'))
+                            ->hidden(fn () => !auth()->user()->can('viewAny', Database::class))
                             ->icon('tabler-database')
                             ->columns(4)
                             ->schema([
@@ -710,7 +710,7 @@ class EditServer extends EditRecord
                                             ->hintAction(
                                                 Action::make('Delete')
                                                     ->label(trans('filament-actions::delete.single.modal.actions.delete.label'))
-                                                    ->authorize(fn (Database $database) => auth()->user()->can('delete database', $database))
+                                                    ->authorize(fn (Database $database) => auth()->user()->can('delete', $database))
                                                     ->color('danger')
                                                     ->icon('tabler-trash')
                                                     ->requiresConfirmation()
@@ -763,7 +763,7 @@ class EditServer extends EditRecord
                                     ->columnSpan(4),
                                 FormActions::make([
                                     Action::make('createDatabase')
-                                        ->authorize(fn () => auth()->user()->can('create database'))
+                                        ->authorize(fn () => auth()->user()->can('create', Database::class))
                                         ->disabled(fn () => DatabaseHost::query()->count() < 1)
                                         ->label(fn () => DatabaseHost::query()->count() < 1 ? trans('admin/server.no_db_hosts') : trans('admin/server.create_database'))
                                         ->color(fn () => DatabaseHost::query()->count() < 1 ? 'danger' : 'primary')
@@ -1065,7 +1065,7 @@ class EditServer extends EditRecord
                     }
                 })
                 ->hidden(fn () => $canForceDelete)
-                ->authorize(fn (Server $server) => auth()->user()->can('delete server', $server)),
+                ->authorize(fn (Server $server) => auth()->user()->can('delete', $server)),
             Actions\Action::make('ForceDelete')
                 ->color('danger')
                 ->label(trans('filament-actions::force-delete.single.label'))
@@ -1082,7 +1082,7 @@ class EditServer extends EditRecord
                     }
                 })
                 ->visible(fn () => $canForceDelete)
-                ->authorize(fn (Server $server) => auth()->user()->can('delete server', $server)),
+                ->authorize(fn (Server $server) => auth()->user()->can('delete', $server)),
             Actions\Action::make('console')
                 ->label(trans('admin/server.console'))
                 ->icon('tabler-terminal')
diff --git a/app/Filament/Admin/Resources/ServerResource/Pages/ListServers.php b/app/Filament/Admin/Resources/ServerResource/Pages/ListServers.php
index 79bdfccbb..1ca2f66d8 100644
--- a/app/Filament/Admin/Resources/ServerResource/Pages/ListServers.php
+++ b/app/Filament/Admin/Resources/ServerResource/Pages/ListServers.php
@@ -68,13 +68,13 @@ class ListServers extends ListRecords
                     ->searchable(),
                 SelectColumn::make('allocation_id')
                     ->label(trans('admin/server.primary_allocation'))
-                    ->hidden(!auth()->user()->can('update server'))
+                    ->hidden(!auth()->user()->can('update server')) // TODO: update to policy check (fn (Server $server) --> $server is empty)
                     ->options(fn (Server $server) => $server->allocations->mapWithKeys(fn ($allocation) => [$allocation->id => $allocation->address]))
                     ->selectablePlaceholder(false)
                     ->sortable(),
                 TextColumn::make('allocation_id_readonly')
                     ->label(trans('admin/server.primary_allocation'))
-                    ->hidden(auth()->user()->can('update server'))
+                    ->hidden(auth()->user()->can('update server')) // TODO: update to policy check (fn (Server $server) --> $server is empty)
                     ->state(fn (Server $server) => $server->allocation->address),
                 TextColumn::make('image')->hidden(),
                 TextColumn::make('backups_count')
diff --git a/app/Filament/Components/Forms/Actions/RotateDatabasePasswordAction.php b/app/Filament/Components/Forms/Actions/RotateDatabasePasswordAction.php
index 5bcab7469..a7eb4ca34 100644
--- a/app/Filament/Components/Forms/Actions/RotateDatabasePasswordAction.php
+++ b/app/Filament/Components/Forms/Actions/RotateDatabasePasswordAction.php
@@ -26,7 +26,7 @@ class RotateDatabasePasswordAction extends Action
 
         $this->icon('tabler-refresh');
 
-        $this->authorize(fn (Database $database) => auth()->user()->can('update database', $database));
+        $this->authorize(fn (Database $database) => auth()->user()->can('update', $database));
 
         $this->modalHeading(trans('admin/databasehost.rotate_password'));
 
diff --git a/app/Filament/Server/Resources/ActivityResource.php b/app/Filament/Server/Resources/ActivityResource.php
index 97b6fb8dc..51edbad3b 100644
--- a/app/Filament/Server/Resources/ActivityResource.php
+++ b/app/Filament/Server/Resources/ActivityResource.php
@@ -68,7 +68,7 @@ class ActivityResource extends Resource
                         return $user;
                     })
                     ->tooltip(fn (ActivityLog $activityLog) => auth()->user()->can('seeIps activityLog') ? $activityLog->ip : '')
-                    ->url(fn (ActivityLog $activityLog) => $activityLog->actor instanceof User && auth()->user()->can('update user') ? EditUser::getUrl(['record' => $activityLog->actor], panel: 'admin') : '')
+                    ->url(fn (ActivityLog $activityLog) => $activityLog->actor instanceof User && auth()->user()->can('update', $activityLog->actor) ? EditUser::getUrl(['record' => $activityLog->actor], panel: 'admin') : '')
                     ->grow(false),
                 DateTimeColumn::make('timestamp')
                     ->since()
@@ -105,7 +105,7 @@ class ActivityResource extends Resource
                                 Action::make('edit')
                                     ->label(trans('filament-actions::edit.single.label'))
                                     ->icon('tabler-edit')
-                                    ->visible(fn (ActivityLog $activityLog) => $activityLog->actor instanceof User && auth()->user()->can('update user'))
+                                    ->visible(fn (ActivityLog $activityLog) => $activityLog->actor instanceof User && auth()->user()->can('update', $activityLog->actor))
                                     ->url(fn (ActivityLog $activityLog) => EditUser::getUrl(['record' => $activityLog->actor], panel: 'admin'))
                             ),
                         DateTimePicker::make('timestamp'),
diff --git a/app/Http/Requests/Api/Client/Servers/Subusers/SubuserRequest.php b/app/Http/Requests/Api/Client/Servers/Subusers/SubuserRequest.php
index cb9933256..b2f0e201e 100644
--- a/app/Http/Requests/Api/Client/Servers/Subusers/SubuserRequest.php
+++ b/app/Http/Requests/Api/Client/Servers/Subusers/SubuserRequest.php
@@ -58,7 +58,7 @@ abstract class SubuserRequest extends ClientApiRequest
         $server = $this->route()->parameter('server');
 
         // 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) {
+        if ($user->can('update', $server) || $user->id === $server->owner_id) {
             return;
         }
 
diff --git a/app/Models/User.php b/app/Models/User.php
index c8d8800b8..a5bdb8305 100644
--- a/app/Models/User.php
+++ b/app/Models/User.php
@@ -265,8 +265,13 @@ class User extends Model implements AuthenticatableContract, AuthorizableContrac
      */
     public function accessibleServers(): Builder
     {
-        if ($this->canned('viewList server')) {
-            return Server::query();
+        if ($this->canned('viewAny', Server::class)) {
+            return Server::select('servers.*')
+                ->leftJoin('subusers', 'subusers.server_id', '=', 'servers.id')
+                ->where(function (Builder $builder) {
+                    $builder->where('servers.owner_id', $this->id)->orWhere('subusers.user_id', $this->id)->orWhereIn('servers.node_id', $this->accessibleNodes()->pluck('id'));
+                })
+                ->distinct('servers.id');
         }
 
         return $this->directAccessibleServers();
@@ -278,8 +283,7 @@ class User extends Model implements AuthenticatableContract, AuthorizableContrac
      */
     public function directAccessibleServers(): Builder
     {
-        return Server::query()
-            ->select('servers.*')
+        return Server::select('servers.*')
             ->leftJoin('subusers', 'subusers.server_id', '=', 'servers.id')
             ->where(function (Builder $builder) {
                 $builder->where('servers.owner_id', $this->id)->orWhere('subusers.user_id', $this->id);
@@ -314,12 +318,12 @@ class User extends Model implements AuthenticatableContract, AuthorizableContrac
 
     protected function checkPermission(Server $server, string $permission = ''): bool
     {
-        if ($this->canned('update server', $server) || $server->owner_id === $this->id) {
+        if ($this->canned('update', $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)) {
+        if ($permission === Permission::ACTION_WEBSOCKET_CONNECT && $this->canned('view', $server)) {
             return true;
         }
 
@@ -434,7 +438,7 @@ class User extends Model implements AuthenticatableContract, AuthorizableContrac
     public function canAccessTenant(Model $tenant): bool
     {
         if ($tenant instanceof Server) {
-            if ($this->canned('view server', $tenant) || $tenant->owner_id === $this->id) {
+            if ($this->canned('view', $tenant) || $tenant->owner_id === $this->id) {
                 return true;
             }
 
diff --git a/app/Policies/ServerPolicy.php b/app/Policies/ServerPolicy.php
index b9fe590f8..539ae2712 100644
--- a/app/Policies/ServerPolicy.php
+++ b/app/Policies/ServerPolicy.php
@@ -21,11 +21,6 @@ class ServerPolicy
             return null;
         }
 
-        // Make sure user can target node of the server
-        if (!$user->canTarget($server->node)) {
-            return false;
-        }
-
         // Owner has full server permissions
         if ($server->owner_id === $user->id) {
             return true;
@@ -37,6 +32,11 @@ class ServerPolicy
             return true;
         }
 
+        // Make sure user can target node of the server
+        if (!$user->canTarget($server->node)) {
+            return false;
+        }
+
         // Return null to let default policies take over
         return null;
     }
diff --git a/app/Services/Servers/GetUserPermissionsService.php b/app/Services/Servers/GetUserPermissionsService.php
index f9d0e31fa..72c74bbf0 100644
--- a/app/Services/Servers/GetUserPermissionsService.php
+++ b/app/Services/Servers/GetUserPermissionsService.php
@@ -16,8 +16,8 @@ class GetUserPermissionsService
      */
     public function handle(Server $server, User $user): array
     {
-        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->isAdmin() && ($user->can('view', $server) || $user->can('update', $server))) {
+            $permissions = $user->can('update', $server) ? ['*'] : ['websocket.connect', 'backup.read'];
 
             $permissions[] = 'admin.websocket.errors';
             $permissions[] = 'admin.websocket.install';