Refactor subuser permissions #1961
Conversation
📝 WalkthroughWalkthroughMigrates permission handling from the removed App\Models\Permission model to a new App\Enums\SubuserPermission enum; updates request signatures, controllers, policies, Filament resources, services, models, factories, transformers, and tests; adds static permission aggregation APIs on Subuser and extends User to accept enum-typed permission checks. Changes
Possibly related PRs
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Boy132
force-pushed
the
boy132/refactor-subuser-permissions
branch
from
ba9844b to
b51ffe4
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (9)
app/Services/Subusers/SubuserUpdateService.php (1)
5-6: Clarify / harden permission type handling inSubuserUpdateService::handleThe updated filter correctly preserves the special case for
WebsocketConnectand delegates all other entries throughuser()->can(...), which fits the newSubuserPermissionflow. One small improvement:
- The docblock states
string[] $permissions, but elsewhere in the codebase tests appear to passSubuserPermissionenum cases directly. In that scenario, the comparison toSubuserPermission::WebsocketConnect->valuewill not match, and the special case will only be honored viacan(...)(i.e. the user must actually have that permission).To keep behavior predictable, consider either:
- Enforcing string inputs at call sites (always passing
->value), or- Supporting both forms here, e.g.:
->filter(function ($permission) use ($server) { $isWebsocket = $permission === SubuserPermission::WebsocketConnect || $permission === SubuserPermission::WebsocketConnect->value; return $isWebsocket || user()?->can($permission, $server); })Also applies to: 21-28
tests/Feature/SettingsControllerTest.php (1)
4-7: Feature tests now consistently exerciseSubuserPermission-based settings accessThe updated tests correctly migrate from Permission constants to
SubuserPermissioncases and keep the expected behavior:
- Rename: unauthorized with only
WebsocketConnect, allowed withWebsocketConnect + SettingsRename.- Description: gated on
SettingsDescriptionand the config flag.- Docker image: unauthorized with only
WebsocketConnect, authorized and validated withStartupDockerImage.- Reinstall: gated on
SettingsReinstall.Since
generateTestAccount()stores the passed array directly as the Subuser’spermissions, just ensure theSubusermodel’s casting forpermissionsis configured to accept enum cases as well as string values so these tests match runtime behavior.Also applies to: 14-26, 28-76, 78-91, 95-135, 138-155, 157-179
tests/Integration/Api/Client/ClientControllerTest.php (1)
158-193: Use enum values when seedingpermissionsfor Subuser recordsHere you’re persisting
permissionsas[SubuserPermission::WebsocketConnect]directly. Elsewhere (e.g.,generateTestAccount()inCreatesTestModels) permissions are normalized to->valuebefore storage. For consistency and to avoid JSON blobs containing enum objects instead of scalar permission keys, consider storing the string values here as well.Subuser::query()->create([ 'user_id' => $users[0]->id, 'server_id' => $servers[1]->id, - 'permissions' => [SubuserPermission::WebsocketConnect], + 'permissions' => [SubuserPermission::WebsocketConnect->value], ]); // … Subuser::query()->create([ 'user_id' => $users[0]->id, 'server_id' => $servers[1]->id, - 'permissions' => [SubuserPermission::WebsocketConnect], + 'permissions' => [SubuserPermission::WebsocketConnect->value], ]); // … Subuser::query()->create([ 'user_id' => $users[0]->id, 'server_id' => $servers[1]->id, - 'permissions' => [SubuserPermission::WebsocketConnect], + 'permissions' => [SubuserPermission::WebsocketConnect->value], ]);Also applies to: 239-243, 274-278
app/Enums/SubuserPermission.php (1)
70-87: Unused$permissionvariable from destructuring.The static analysis correctly flags that
$permissionis destructured but never used. Since only the group is needed, consider simplifying.public function getIcon(): ?string { - [$group, $permission] = $this->split(); + [$group] = $this->split(); return match ($group) {app/Http/Requests/Api/Client/Servers/Backups/RenameBackupRequest.php (1)
10-12: Migration correct, but semantically unusual permission choice.The migration to
SubuserPermission::BackupDeleteis consistent with the previous implementation (preserving existing behavior as intended). However, requiring "delete" permission for a rename operation is semantically unusual—rename is typically an "update" operation. Since there's noBackupUpdatepermission defined in the enum, this may warrant adding one in a future iteration, or it may be an intentional design choice to keep backup permissions simple.app/Http/Controllers/Api/Client/Servers/SubuserController.php (1)
136-149: Tighten typing and normalization in getCleanedPermissions()The implementation effectively returns a list of permission-key strings, but the PHPDoc is overly loose and keys may remain non-sequential:
- Docblock says
array<array-key, mixed>while the method returnsstring[].- Returning a reindexed list avoids any surprises from preserved request keys.
You could make this more explicit and normalized:
- /** - * @return array<array-key, mixed> - */ - protected function getCleanedPermissions(Request $request): array - { - return collect($request->input('permissions') ?? []) - ->intersect(Subuser::allPermissionKeys()) - ->push(SubuserPermission::WebsocketConnect->value) - ->unique() - ->toArray(); - } + /** + * @return string[] + */ + protected function getCleanedPermissions(Request $request): array + { + return collect($request->input('permissions') ?? []) + ->intersect(Subuser::allPermissionKeys()) + ->push(SubuserPermission::WebsocketConnect->value) + ->unique() + ->values() + ->toArray(); + }app/Models/Subuser.php (2)
36-47: Fix inaccurate PHPDoc for $customPermissions and registerCustomPermission()The documented shapes don’t match the actual data structure:
$customPermissionsis documented asarray<string, array<string>>, but you’re storing a list of associative arrays with keysname,icon, andpermissions.$permissionsinregisterCustomPermission()is documented asarray<string, array<string>>, but appears to be a simple list of permission slugs (string[]).Consider tightening these to better reflect reality, e.g.:
- /** @var array<string, array<string>> */ - protected static array $customPermissions = []; + /** @var list<array{name: string, icon: string, permissions: string[]}> */ + protected static array $customPermissions = []; - /** @param array<string, array<string>> $permissions */ - public static function registerCustomPermission(string $name, string $icon, array $permissions): void + /** @param string[] $permissions */ + public static function registerCustomPermission(string $name, string $icon, array $permissions): voidThis will help static analysis and IDEs understand the structure more accurately.
87-125: Permission aggregation helpers look correct; consider future-proofing if usage grows
allPermissionData()andallPermissionKeys()correctly derive grouped permission metadata and flattened keys fromSubuserPermission::cases()plus$customPermissions, anddoesPermissionExist()safely validates keys viaallPermissionKeys().No functional issues stand out here. If these helpers end up in hot paths (e.g., called many times per request), you might later consider memoizing the computed arrays and invalidating them from
registerCustomPermission(), but that’s an optimization, not a blocker.app/Filament/Server/Resources/Databases/DatabaseResource.php (1)
213-236: Resource-level permission checks align with SubuserPermission; handle unused $record warningsThe
canViewAny/canView/canCreate/canEdit/canDeletemethods now consistently delegate to:
DatabaseReadfor view/viewAnyDatabaseCreatefor createDatabaseUpdatefor editDatabaseDeletefor deleteThis matches the intended enum-based permission scheme.
PHPMD’s “unused parameter
$record” warnings forcanView,canEdit, andcanDeletestem from Filament’s required method signature. If these warnings are noisy in CI, consider one of:
- Renaming the parameter to
$_recordto signal intentional non-use, or- Adding an appropriate PHPMD suppression annotation for these methods.
Functionally, the implementation is fine as-is.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (107)
app/Contracts/Http/ClientPermissionsRequest.php(1 hunks)app/Enums/SubuserPermission.php(1 hunks)app/Extensions/Features/Schemas/GSLTokenSchema.php(2 hunks)app/Extensions/Features/Schemas/JavaVersionSchema.php(2 hunks)app/Filament/Admin/Resources/Roles/Pages/CreateRole.php(2 hunks)app/Filament/App/Resources/Servers/Pages/ListServers.php(2 hunks)app/Filament/Components/Actions/ExportScheduleAction.php(2 hunks)app/Filament/Components/Actions/ImportScheduleAction.php(2 hunks)app/Filament/Server/Pages/Console.php(3 hunks)app/Filament/Server/Pages/Settings.php(7 hunks)app/Filament/Server/Pages/Startup.php(5 hunks)app/Filament/Server/Resources/Activities/ActivityResource.php(2 hunks)app/Filament/Server/Resources/Allocations/AllocationResource.php(6 hunks)app/Filament/Server/Resources/Backups/BackupResource.php(7 hunks)app/Filament/Server/Resources/Databases/DatabaseResource.php(4 hunks)app/Filament/Server/Resources/Files/FileResource.php(2 hunks)app/Filament/Server/Resources/Files/Pages/DownloadFiles.php(2 hunks)app/Filament/Server/Resources/Files/Pages/EditFiles.php(4 hunks)app/Filament/Server/Resources/Files/Pages/ListFiles.php(17 hunks)app/Filament/Server/Resources/Schedules/Pages/ViewSchedule.php(2 hunks)app/Filament/Server/Resources/Schedules/ScheduleResource.php(2 hunks)app/Filament/Server/Resources/Users/UserResource.php(8 hunks)app/Filament/Server/Widgets/ServerConsole.php(3 hunks)app/Http/Controllers/Api/Client/ClientController.php(2 hunks)app/Http/Controllers/Api/Client/Servers/ActivityLogController.php(2 hunks)app/Http/Controllers/Api/Client/Servers/BackupController.php(7 hunks)app/Http/Controllers/Api/Client/Servers/ScheduleTaskController.php(2 hunks)app/Http/Controllers/Api/Client/Servers/SubuserController.php(4 hunks)app/Http/Controllers/Api/Client/Servers/WebsocketController.php(2 hunks)app/Http/Controllers/Api/Remote/SftpAuthenticationController.php(2 hunks)app/Http/Requests/Api/Client/Servers/Backups/RenameBackupRequest.php(1 hunks)app/Http/Requests/Api/Client/Servers/Backups/RestoreBackupRequest.php(1 hunks)app/Http/Requests/Api/Client/Servers/Backups/StoreBackupRequest.php(1 hunks)app/Http/Requests/Api/Client/Servers/Databases/DeleteDatabaseRequest.php(1 hunks)app/Http/Requests/Api/Client/Servers/Databases/GetDatabasesRequest.php(1 hunks)app/Http/Requests/Api/Client/Servers/Databases/RotatePasswordRequest.php(1 hunks)app/Http/Requests/Api/Client/Servers/Databases/StoreDatabaseRequest.php(2 hunks)app/Http/Requests/Api/Client/Servers/Files/ChmodFilesRequest.php(1 hunks)app/Http/Requests/Api/Client/Servers/Files/CompressFilesRequest.php(1 hunks)app/Http/Requests/Api/Client/Servers/Files/CopyFileRequest.php(1 hunks)app/Http/Requests/Api/Client/Servers/Files/CreateFolderRequest.php(1 hunks)app/Http/Requests/Api/Client/Servers/Files/DecompressFilesRequest.php(2 hunks)app/Http/Requests/Api/Client/Servers/Files/DeleteFileRequest.php(1 hunks)app/Http/Requests/Api/Client/Servers/Files/GetFileContentsRequest.php(2 hunks)app/Http/Requests/Api/Client/Servers/Files/ListFilesRequest.php(1 hunks)app/Http/Requests/Api/Client/Servers/Files/PullFileRequest.php(1 hunks)app/Http/Requests/Api/Client/Servers/Files/RenameFileRequest.php(1 hunks)app/Http/Requests/Api/Client/Servers/Files/UploadFileRequest.php(1 hunks)app/Http/Requests/Api/Client/Servers/Files/WriteFileContentRequest.php(2 hunks)app/Http/Requests/Api/Client/Servers/Network/DeleteAllocationRequest.php(1 hunks)app/Http/Requests/Api/Client/Servers/Network/GetNetworkRequest.php(1 hunks)app/Http/Requests/Api/Client/Servers/Network/NewAllocationRequest.php(1 hunks)app/Http/Requests/Api/Client/Servers/Network/UpdateAllocationRequest.php(1 hunks)app/Http/Requests/Api/Client/Servers/Schedules/DeleteScheduleRequest.php(1 hunks)app/Http/Requests/Api/Client/Servers/Schedules/StoreScheduleRequest.php(1 hunks)app/Http/Requests/Api/Client/Servers/Schedules/StoreTaskRequest.php(2 hunks)app/Http/Requests/Api/Client/Servers/Schedules/TriggerScheduleRequest.php(1 hunks)app/Http/Requests/Api/Client/Servers/Schedules/UpdateScheduleRequest.php(1 hunks)app/Http/Requests/Api/Client/Servers/Schedules/ViewScheduleRequest.php(2 hunks)app/Http/Requests/Api/Client/Servers/SendCommandRequest.php(1 hunks)app/Http/Requests/Api/Client/Servers/SendPowerRequest.php(1 hunks)app/Http/Requests/Api/Client/Servers/Settings/DescriptionServerRequest.php(2 hunks)app/Http/Requests/Api/Client/Servers/Settings/ReinstallServerRequest.php(1 hunks)app/Http/Requests/Api/Client/Servers/Settings/RenameServerRequest.php(2 hunks)app/Http/Requests/Api/Client/Servers/Settings/SetDockerImageRequest.php(1 hunks)app/Http/Requests/Api/Client/Servers/Startup/GetStartupRequest.php(1 hunks)app/Http/Requests/Api/Client/Servers/Startup/UpdateStartupVariableRequest.php(1 hunks)app/Http/Requests/Api/Client/Servers/Subusers/DeleteSubuserRequest.php(1 hunks)app/Http/Requests/Api/Client/Servers/Subusers/GetSubuserRequest.php(1 hunks)app/Http/Requests/Api/Client/Servers/Subusers/StoreSubuserRequest.php(1 hunks)app/Http/Requests/Api/Client/Servers/Subusers/UpdateSubuserRequest.php(1 hunks)app/Models/Permission.php(0 hunks)app/Models/Subuser.php(3 hunks)app/Models/User.php(3 hunks)app/Policies/ServerPolicy.php(2 hunks)app/Services/Servers/GetUserPermissionsService.php(2 hunks)app/Services/Subusers/SubuserCreationService.php(2 hunks)app/Services/Subusers/SubuserUpdateService.php(2 hunks)app/Transformers/Api/Client/DatabaseTransformer.php(2 hunks)app/Transformers/Api/Client/ServerTransformer.php(5 hunks)database/Factories/PermissionFactory.php(0 hunks)database/Factories/SubuserFactory.php(2 hunks)tests/Feature/SettingsControllerTest.php(10 hunks)tests/Filament/Admin/ListEggsTest.php(0 hunks)tests/Filament/Admin/ListNodesTest.php(0 hunks)tests/Integration/Api/Client/ClientControllerTest.php(7 hunks)tests/Integration/Api/Client/Server/Allocation/CreateNewAllocationTest.php(3 hunks)tests/Integration/Api/Client/Server/Allocation/DeleteAllocationTest.php(3 hunks)tests/Integration/Api/Client/Server/Backup/DeleteBackupTest.php(3 hunks)tests/Integration/Api/Client/Server/CommandControllerTest.php(3 hunks)tests/Integration/Api/Client/Server/NetworkAllocationControllerTest.php(4 hunks)tests/Integration/Api/Client/Server/PowerControllerTest.php(4 hunks)tests/Integration/Api/Client/Server/ResourceUtilizationControllerTest.php(2 hunks)tests/Integration/Api/Client/Server/Schedule/CreateServerScheduleTest.php(3 hunks)tests/Integration/Api/Client/Server/Schedule/DeleteServerScheduleTest.php(3 hunks)tests/Integration/Api/Client/Server/Schedule/ExecuteScheduleTest.php(3 hunks)tests/Integration/Api/Client/Server/Schedule/GetServerSchedulesTest.php(3 hunks)tests/Integration/Api/Client/Server/Schedule/UpdateServerScheduleTest.php(3 hunks)tests/Integration/Api/Client/Server/ScheduleTask/CreateServerScheduleTaskTest.php(3 hunks)tests/Integration/Api/Client/Server/ScheduleTask/DeleteScheduleTaskTest.php(2 hunks)tests/Integration/Api/Client/Server/SettingsControllerTest.php(4 hunks)tests/Integration/Api/Client/Server/Startup/GetStartupAndVariablesTest.php(3 hunks)tests/Integration/Api/Client/Server/Startup/UpdateStartupVariableTest.php(3 hunks)tests/Integration/Api/Client/Server/Subuser/CreateServerSubuserTest.php(10 hunks)tests/Integration/Api/Client/Server/Subuser/DeleteSubuserTest.php(3 hunks)tests/Integration/Api/Client/Server/Subuser/UpdateSubuserTest.php(5 hunks)tests/Integration/Api/Client/Server/WebsocketControllerTest.php(4 hunks)
⛔ Files not processed due to max files limit (2)
- tests/Integration/Api/Remote/SftpAuthenticationControllerTest.php
- tests/Traits/Integration/CreatesTestModels.php
💤 Files with no reviewable changes (4)
- database/Factories/PermissionFactory.php
- app/Models/Permission.php
- tests/Filament/Admin/ListNodesTest.php
- tests/Filament/Admin/ListEggsTest.php
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-12T17:33:57.388Z
Learnt from: lancepioch
Repo: pelican-dev/panel PR: 1595
File: app/Services/Servers/ServerCreationService.php:182-184
Timestamp: 2025-08-12T17:33:57.388Z
Learning: In the Pelican Panel codebase, when updating allocations to assign them to servers, the preferred pattern is to use whereNull('server_id'), lockForUpdate(), and direct property assignment with save() rather than mass update methods, to prevent race conditions and mass-assignment issues while ensuring model events fire properly.
Applied to files:
app/Filament/Server/Resources/Allocations/AllocationResource.php
📚 Learning: 2025-11-05T22:21:31.863Z
Learnt from: notAreYouScared
Repo: pelican-dev/panel PR: 1865
File: app/Filament/Admin/Resources/Nodes/Pages/EditNode.php:682-682
Timestamp: 2025-11-05T22:21:31.863Z
Learning: In app/Filament/Admin/Resources/Nodes/Pages/EditNode.php, the diagnostics tab's upload action intentionally does not use the iconButton() modifier, while the pull action does. This UI difference is intentional.
Applied to files:
app/Filament/Server/Resources/Files/Pages/ListFiles.php
🧬 Code graph analysis (66)
tests/Integration/Api/Client/Server/ScheduleTask/CreateServerScheduleTaskTest.php (1)
tests/Pest.php (1)
generateTestAccount(127-145)
app/Http/Requests/Api/Client/Servers/Network/NewAllocationRequest.php (1)
app/Http/Requests/Api/Client/ClientApiRequest.php (1)
ClientApiRequest(9-30)
app/Filament/Server/Resources/Allocations/AllocationResource.php (7)
app/helpers.php (1)
user(127-130)app/Models/Server.php (1)
allocation(280-283)app/Filament/Server/Resources/Backups/BackupResource.php (2)
canCreate(306-309)canDelete(311-314)app/Filament/Server/Resources/Databases/DatabaseResource.php (3)
canCreate(223-226)canEdit(228-231)canDelete(233-236)app/Filament/Server/Resources/Files/FileResource.php (3)
canCreate(38-41)canEdit(43-46)canDelete(48-51)app/Filament/Server/Resources/Schedules/ScheduleResource.php (3)
canCreate(72-75)canEdit(77-80)canDelete(82-85)app/Filament/Server/Resources/Users/UserResource.php (3)
canCreate(72-75)canEdit(77-80)canDelete(82-85)
app/Http/Requests/Api/Client/Servers/Files/ChmodFilesRequest.php (1)
app/Http/Requests/Api/Client/ClientApiRequest.php (1)
ClientApiRequest(9-30)
tests/Integration/Api/Client/Server/Schedule/UpdateServerScheduleTest.php (1)
tests/Pest.php (1)
generateTestAccount(127-145)
app/Http/Requests/Api/Client/Servers/Schedules/TriggerScheduleRequest.php (1)
app/Http/Requests/Api/Client/ClientApiRequest.php (1)
ClientApiRequest(9-30)
app/Filament/Components/Actions/ExportScheduleAction.php (1)
app/helpers.php (1)
user(127-130)
app/Http/Requests/Api/Client/Servers/SendCommandRequest.php (1)
app/Http/Requests/Api/Client/ClientApiRequest.php (1)
ClientApiRequest(9-30)
app/Http/Requests/Api/Client/Servers/Subusers/GetSubuserRequest.php (1)
app/Http/Requests/Api/Client/Servers/Subusers/SubuserRequest.php (1)
SubuserRequest(14-77)
app/Http/Requests/Api/Client/Servers/Network/DeleteAllocationRequest.php (1)
app/Http/Requests/Api/Client/ClientApiRequest.php (1)
ClientApiRequest(9-30)
tests/Feature/SettingsControllerTest.php (1)
tests/Pest.php (1)
generateTestAccount(127-145)
tests/Integration/Api/Client/Server/NetworkAllocationControllerTest.php (3)
tests/Pest.php (1)
generateTestAccount(127-145)tests/Integration/Api/Client/ClientApiIntegrationTestCase.php (1)
link(29-52)app/Models/Server.php (1)
allocation(280-283)
tests/Integration/Api/Client/Server/WebsocketControllerTest.php (1)
tests/Traits/Integration/CreatesTestModels.php (1)
generateTestAccount(72-90)
app/Extensions/Features/Schemas/GSLTokenSchema.php (3)
app/helpers.php (1)
user(127-130)app/Models/User.php (1)
can(362-371)app/Models/Schedule.php (1)
server(141-144)
tests/Integration/Api/Client/Server/Subuser/UpdateSubuserTest.php (1)
tests/Traits/Integration/CreatesTestModels.php (1)
generateTestAccount(72-90)
app/Services/Subusers/SubuserUpdateService.php (2)
app/helpers.php (1)
user(127-130)app/Models/User.php (1)
can(362-371)
app/Http/Requests/Api/Client/Servers/Settings/RenameServerRequest.php (1)
app/Http/Requests/Api/Client/Servers/Settings/DescriptionServerRequest.php (1)
permission(16-19)
tests/Integration/Api/Client/Server/Startup/GetStartupAndVariablesTest.php (1)
tests/Pest.php (1)
generateTestAccount(127-145)
tests/Integration/Api/Client/Server/Allocation/CreateNewAllocationTest.php (1)
tests/Pest.php (1)
generateTestAccount(127-145)
app/Http/Requests/Api/Client/Servers/Databases/DeleteDatabaseRequest.php (1)
app/Http/Requests/Api/Client/ClientApiRequest.php (1)
ClientApiRequest(9-30)
app/Filament/Server/Resources/Files/FileResource.php (6)
app/helpers.php (1)
user(127-130)app/Filament/Server/Resources/Allocations/AllocationResource.php (3)
canCreate(130-133)canEdit(135-138)canDelete(140-143)app/Filament/Server/Resources/Backups/BackupResource.php (2)
canCreate(306-309)canDelete(311-314)app/Filament/Server/Resources/Databases/DatabaseResource.php (3)
canCreate(223-226)canEdit(228-231)canDelete(233-236)app/Filament/Server/Resources/Schedules/ScheduleResource.php (3)
canCreate(72-75)canEdit(77-80)canDelete(82-85)app/Filament/Server/Resources/Users/UserResource.php (3)
canCreate(72-75)canEdit(77-80)canDelete(82-85)
app/Filament/Server/Widgets/ServerConsole.php (1)
app/Models/User.php (1)
can(362-371)
tests/Integration/Api/Client/Server/Startup/UpdateStartupVariableTest.php (1)
tests/Pest.php (1)
generateTestAccount(127-145)
app/Http/Requests/Api/Client/Servers/Network/GetNetworkRequest.php (1)
app/Http/Requests/Api/Client/ClientApiRequest.php (1)
ClientApiRequest(9-30)
app/Http/Requests/Api/Client/Servers/Network/UpdateAllocationRequest.php (2)
app/Http/Requests/Api/Client/ClientApiRequest.php (1)
ClientApiRequest(9-30)app/Models/Allocation.php (1)
Allocation(48-141)
app/Http/Requests/Api/Client/Servers/Files/RenameFileRequest.php (1)
app/Http/Requests/Api/Client/ClientApiRequest.php (1)
ClientApiRequest(9-30)
app/Filament/Server/Resources/Activities/ActivityResource.php (1)
app/helpers.php (1)
user(127-130)
app/Http/Requests/Api/Client/Servers/Backups/StoreBackupRequest.php (2)
app/Http/Requests/Api/Client/ClientApiRequest.php (1)
ClientApiRequest(9-30)app/Http/Requests/Api/Client/Servers/Subusers/StoreSubuserRequest.php (1)
permission(9-12)
app/Http/Requests/Api/Client/Servers/Files/DeleteFileRequest.php (1)
app/Http/Requests/Api/Client/ClientApiRequest.php (1)
ClientApiRequest(9-30)
tests/Integration/Api/Client/Server/Schedule/DeleteServerScheduleTest.php (1)
tests/Pest.php (1)
generateTestAccount(127-145)
app/Filament/Server/Resources/Users/UserResource.php (2)
app/Models/Subuser.php (3)
Subuser(24-126)user(82-85)allPermissionData(88-106)app/Models/User.php (1)
can(362-371)
tests/Integration/Api/Client/Server/Schedule/ExecuteScheduleTest.php (1)
tests/Pest.php (1)
generateTestAccount(127-145)
app/Http/Controllers/Api/Client/Servers/BackupController.php (1)
app/Models/User.php (1)
can(362-371)
tests/Integration/Api/Client/Server/Schedule/CreateServerScheduleTest.php (1)
tests/Pest.php (1)
generateTestAccount(127-145)
app/Http/Requests/Api/Client/Servers/Files/CopyFileRequest.php (1)
app/Http/Requests/Api/Client/ClientApiRequest.php (1)
ClientApiRequest(9-30)
app/Http/Requests/Api/Client/Servers/Schedules/DeleteScheduleRequest.php (1)
app/Http/Requests/Api/Client/Servers/Schedules/ViewScheduleRequest.php (1)
ViewScheduleRequest(12-43)
app/Models/User.php (3)
app/Models/Subuser.php (3)
server(74-77)Subuser(24-126)doesPermissionExist(118-125)app/Http/Requests/Api/Client/Servers/Backups/StoreBackupRequest.php (1)
permission(10-13)app/Http/Requests/Api/Client/Servers/Subusers/StoreSubuserRequest.php (1)
permission(9-12)
app/Http/Requests/Api/Client/Servers/SendPowerRequest.php (1)
app/Http/Requests/Api/Client/ClientApiRequest.php (1)
ClientApiRequest(9-30)
app/Http/Requests/Api/Client/Servers/Settings/ReinstallServerRequest.php (1)
app/Http/Requests/Api/Client/ClientApiRequest.php (1)
ClientApiRequest(9-30)
app/Http/Requests/Api/Client/Servers/Settings/DescriptionServerRequest.php (1)
app/Http/Requests/Api/Client/Servers/Settings/RenameServerRequest.php (1)
permission(17-20)
app/Transformers/Api/Client/DatabaseTransformer.php (2)
app/Models/User.php (1)
can(362-371)app/Models/Database.php (1)
server(84-87)
app/Http/Controllers/Api/Remote/SftpAuthenticationController.php (1)
app/Http/Controllers/Api/Client/ClientController.php (1)
permissions(85-93)
app/Http/Controllers/Api/Client/Servers/SubuserController.php (3)
app/Services/Subusers/SubuserCreationService.php (1)
handle(39-77)app/Services/Subusers/SubuserUpdateService.php (1)
handle(21-61)app/Models/Subuser.php (4)
server(74-77)user(82-85)Subuser(24-126)allPermissionKeys(109-116)
tests/Integration/Api/Client/ClientControllerTest.php (3)
app/Models/Subuser.php (4)
Subuser(24-126)allPermissionKeys(109-116)user(82-85)server(74-77)tests/Traits/Integration/CreatesTestModels.php (1)
generateTestAccount(72-90)tests/Pest.php (1)
generateTestAccount(127-145)
app/Extensions/Features/Schemas/JavaVersionSchema.php (2)
app/helpers.php (1)
user(127-130)app/Models/User.php (1)
can(362-371)
app/Http/Requests/Api/Client/Servers/Subusers/UpdateSubuserRequest.php (1)
app/Http/Requests/Api/Client/Servers/Subusers/SubuserRequest.php (1)
SubuserRequest(14-77)
app/Filament/Server/Resources/Files/Pages/ListFiles.php (4)
app/helpers.php (1)
user(127-130)app/Filament/Server/Resources/Databases/DatabaseResource.php (1)
canEdit(228-231)app/Filament/Server/Resources/Files/FileResource.php (1)
canEdit(43-46)app/Filament/Server/Resources/Schedules/ScheduleResource.php (1)
canEdit(77-80)
app/Filament/Server/Pages/Console.php (2)
app/Http/Requests/Api/Client/Servers/Subusers/SubuserRequest.php (1)
authorize(23-46)app/Models/User.php (1)
can(362-371)
app/Http/Requests/Api/Client/Servers/Files/UploadFileRequest.php (1)
app/Http/Requests/Api/Client/ClientApiRequest.php (1)
ClientApiRequest(9-30)
tests/Integration/Api/Client/Server/SettingsControllerTest.php (2)
app/Models/Server.php (1)
user(262-265)tests/Pest.php (1)
generateTestAccount(127-145)
tests/Integration/Api/Client/Server/Schedule/GetServerSchedulesTest.php (1)
tests/Pest.php (1)
generateTestAccount(127-145)
app/Filament/Components/Actions/ImportScheduleAction.php (1)
app/helpers.php (1)
user(127-130)
app/Http/Requests/Api/Client/Servers/Databases/RotatePasswordRequest.php (1)
app/Http/Requests/Api/Client/ClientApiRequest.php (1)
ClientApiRequest(9-30)
app/Filament/Server/Resources/Schedules/ScheduleResource.php (6)
app/helpers.php (1)
user(127-130)app/Filament/Server/Resources/Allocations/AllocationResource.php (3)
canCreate(130-133)canEdit(135-138)canDelete(140-143)app/Filament/Server/Resources/Backups/BackupResource.php (2)
canCreate(306-309)canDelete(311-314)app/Filament/Server/Resources/Databases/DatabaseResource.php (3)
canCreate(223-226)canEdit(228-231)canDelete(233-236)app/Filament/Server/Resources/Files/FileResource.php (3)
canCreate(38-41)canEdit(43-46)canDelete(48-51)app/Filament/Server/Resources/Users/UserResource.php (3)
canCreate(72-75)canEdit(77-80)canDelete(82-85)
tests/Integration/Api/Client/Server/ResourceUtilizationControllerTest.php (1)
tests/Traits/Integration/CreatesTestModels.php (1)
generateTestAccount(72-90)
app/Filament/Server/Resources/Backups/BackupResource.php (6)
app/helpers.php (1)
user(127-130)app/Filament/Server/Resources/Allocations/AllocationResource.php (2)
canCreate(130-133)canDelete(140-143)app/Filament/Server/Resources/Databases/DatabaseResource.php (2)
canCreate(223-226)canDelete(233-236)app/Filament/Server/Resources/Files/FileResource.php (2)
canCreate(38-41)canDelete(48-51)app/Filament/Server/Resources/Schedules/ScheduleResource.php (2)
canCreate(72-75)canDelete(82-85)app/Filament/Server/Resources/Users/UserResource.php (2)
canCreate(72-75)canDelete(82-85)
app/Policies/ServerPolicy.php (1)
app/Models/Subuser.php (2)
Subuser(24-126)doesPermissionExist(118-125)
app/Models/Subuser.php (2)
app/Http/Controllers/Api/Client/ClientController.php (1)
permissions(85-93)app/Enums/SubuserPermission.php (2)
split(60-63)isHidden(65-68)
app/Http/Requests/Api/Client/Servers/Files/CreateFolderRequest.php (1)
app/Http/Requests/Api/Client/ClientApiRequest.php (1)
ClientApiRequest(9-30)
app/Http/Requests/Api/Client/Servers/Backups/RestoreBackupRequest.php (1)
app/Http/Requests/Api/Client/ClientApiRequest.php (1)
ClientApiRequest(9-30)
app/Http/Requests/Api/Client/Servers/Files/ListFilesRequest.php (1)
app/Http/Requests/Api/Client/ClientApiRequest.php (1)
ClientApiRequest(9-30)
app/Http/Requests/Api/Client/Servers/Subusers/StoreSubuserRequest.php (2)
app/Http/Requests/Api/Client/Servers/Subusers/SubuserRequest.php (1)
SubuserRequest(14-77)app/Http/Requests/Api/Client/Servers/Backups/StoreBackupRequest.php (1)
permission(10-13)
tests/Integration/Api/Client/Server/Allocation/DeleteAllocationTest.php (2)
app/Models/Allocation.php (1)
server(129-132)tests/Pest.php (1)
generateTestAccount(127-145)
app/Http/Requests/Api/Client/Servers/Backups/RenameBackupRequest.php (1)
app/Http/Requests/Api/Client/ClientApiRequest.php (1)
ClientApiRequest(9-30)
app/Http/Requests/Api/Client/Servers/Databases/GetDatabasesRequest.php (1)
app/Http/Requests/Api/Client/ClientApiRequest.php (1)
ClientApiRequest(9-30)
app/Http/Requests/Api/Client/Servers/Startup/GetStartupRequest.php (1)
app/Http/Requests/Api/Client/ClientApiRequest.php (1)
ClientApiRequest(9-30)
🪛 PHPMD (2.15.0)
tests/Integration/Api/Client/Server/CommandControllerTest.php
55-55: Avoid unused local variables such as '$user'. (undefined)
(UnusedLocalVariable)
app/Filament/Server/Resources/Allocations/AllocationResource.php
135-135: Avoid unused parameters such as '$record'. (undefined)
(UnusedFormalParameter)
140-140: Avoid unused parameters such as '$record'. (undefined)
(UnusedFormalParameter)
app/Enums/SubuserPermission.php
72-72: Avoid unused local variables such as '$permission'. (undefined)
(UnusedLocalVariable)
app/Filament/Server/Resources/Files/FileResource.php
43-43: Avoid unused parameters such as '$record'. (undefined)
(UnusedFormalParameter)
48-48: Avoid unused parameters such as '$record'. (undefined)
(UnusedFormalParameter)
app/Filament/Server/Resources/Users/UserResource.php
77-77: Avoid unused parameters such as '$record'. (undefined)
(UnusedFormalParameter)
82-82: Avoid unused parameters such as '$record'. (undefined)
(UnusedFormalParameter)
app/Filament/Server/Resources/Databases/DatabaseResource.php
218-218: Avoid unused parameters such as '$record'. (undefined)
(UnusedFormalParameter)
228-228: Avoid unused parameters such as '$record'. (undefined)
(UnusedFormalParameter)
233-233: Avoid unused parameters such as '$record'. (undefined)
(UnusedFormalParameter)
app/Filament/Server/Resources/Schedules/ScheduleResource.php
77-77: Avoid unused parameters such as '$record'. (undefined)
(UnusedFormalParameter)
82-82: Avoid unused parameters such as '$record'. (undefined)
(UnusedFormalParameter)
app/Filament/Server/Resources/Backups/BackupResource.php
311-311: Avoid unused parameters such as '$record'. (undefined)
(UnusedFormalParameter)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/Filament/Server/Resources/Users/UserResource.php (1)
5-10: Enum‑based permission checks look consistent with the new modelImporting
SubuserPermission/Subuserand swappingcanViewAny/canCreate/canEdit/canDeletetouser()?->can(SubuserPermission::User*, Filament::getTenant())matches the rest of the enum‑based refactor and keeps the resource aligned with the newUser*permission names. The$recordparameter incanEdit/canDeleteremains unused but is required by Filament’s static signature, so PHPMD’s unused‑parameter warning can be safely ignored or suppressed.Also applies to: 67-85
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/Contracts/Http/ClientPermissionsRequest.php(1 hunks)app/Filament/Server/Resources/Users/UserResource.php(8 hunks)app/Http/Controllers/Api/Client/Servers/SubuserController.php(4 hunks)app/Models/Subuser.php(3 hunks)database/Factories/SubuserFactory.php(2 hunks)tests/Integration/Api/Client/ClientControllerTest.php(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/Integration/Api/Client/ClientControllerTest.php
- app/Contracts/Http/ClientPermissionsRequest.php
🧰 Additional context used
🧬 Code graph analysis (3)
app/Http/Controllers/Api/Client/Servers/SubuserController.php (2)
app/Http/Controllers/Api/Client/ClientController.php (1)
permissions(85-93)app/Models/Subuser.php (4)
server(83-86)user(91-94)Subuser(24-148)allPermissionKeys(131-138)
app/Filament/Server/Resources/Users/UserResource.php (8)
app/Models/Server.php (1)
Server(134-525)app/Models/Subuser.php (4)
Subuser(24-148)user(91-94)allPermissionData(97-128)server(83-86)app/helpers.php (1)
user(127-130)app/Models/User.php (1)
can(362-371)app/Filament/Server/Resources/Files/FileResource.php (3)
canCreate(38-41)canEdit(43-46)canDelete(48-51)app/Filament/Server/Resources/Databases/DatabaseResource.php (3)
canCreate(223-226)canEdit(228-231)canDelete(233-236)app/Filament/Server/Resources/Backups/BackupResource.php (2)
canCreate(306-309)canDelete(311-314)app/Filament/Server/Resources/Schedules/ScheduleResource.php (3)
canCreate(72-75)canEdit(77-80)canDelete(82-85)
app/Models/Subuser.php (2)
app/Http/Controllers/Api/Client/ClientController.php (1)
permissions(85-93)app/Enums/SubuserPermission.php (2)
split(60-63)isHidden(65-68)
🪛 PHPMD (2.15.0)
app/Filament/Server/Resources/Users/UserResource.php
77-77: Avoid unused parameters such as '$record'. (undefined)
(UnusedFormalParameter)
82-82: Avoid unused parameters such as '$record'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (7)
database/Factories/SubuserFactory.php (1)
5-26: Enum-backed permission usage looks correct and backward‑compatibleSwitching the factory default from the old
Permissionconstant toSubuserPermission::WebsocketConnect->valuekeeps the stored value as the enum backing value, which should preserve existing behavior and data format while aligning with the new enum-based API. No issues from this change in the factory.app/Filament/Server/Resources/Users/UserResource.php (2)
95-125: UsingSubuser::allPermissionData()correctly drives dynamic permission tabsIterating over
Subuser::allPermissionData()and skipping$data['hidden']ensures hidden/internal permissions (e.g., websocket connect) don’t surface in the UI while still allowing custom permission groups to appear. TheSection::make()->contained(false)change is purely presentational and doesn’t affect behavior.
160-173: WebsocketConnect auto‑grant is consistent, but double‑check that it’s intendedBoth the edit and invite flows now:
- Authorize using
SubuserPermission::UserUpdate/UserCreate, and- Build
$permissionsfrom the selected groups and alwayspush(SubuserPermission::WebsocketConnect->value)before deduping.This keeps Filament behavior consistent with the API controller (which also forces WebsocketConnect) and guarantees that any subuser with UI‑assigned permissions can connect via websockets. Please confirm that granting
WebsocketConnecteven when no other permissions are selected is the desired behavior.Also applies to: 243-249, 296-303
app/Http/Controllers/Api/Client/Servers/SubuserController.php (2)
83-96: Store flow now logs persisted permissions and returns the created subuserUsing
$permissions = $this->getCleanedPermissions($request)and then logging['email' => $email, 'permissions' => $subuser->permissions]ensures the activity log reflects the actual saved state instead of the raw request payload, and returning the transformed$subuserkeeps the API response coherent with that change.
109-119: Permission cleaning logic is sound; confirm always‑on WebsocketConnect
getCleanedPermissions():collect($request->input('permissions') ?? []) ->intersect(Subuser::allPermissionKeys()) ->push(SubuserPermission::WebsocketConnect->value) ->unique() ->values() ->toArray();This:
- Discards any unknown permission strings,
- Guarantees a well‑formed
string[]for the services, and- Always adds
WebsocketConnect(even if the client omits it or sends no permissions).That behavior matches the Filament resource and centralizes sanitization for both
storeandupdate. Please confirm that granting websocket access to every created/updated subuser is intentional from an API perspective.Also applies to: 136-151
app/Models/Subuser.php (2)
36-56: Custom permissions registry API is clear and composable
$customPermissionsplusregisterCustomPermissions($name, array $permissions, ?string $icon = null, ?bool $hidden = null)gives extensions a straightforward way to add/augment permission groups and reuse the same structure as enum‑derived groups. Merging permissions across multiple calls is handy for plugins that register in stages.
130-147: Permission key helpers are cohesive and match the new enum model
allPermissionKeys()composing"$data['name'].$permission"fromallPermissionData()anddoesPermissionExist(string|SubuserPermission $permission)normalizing enums to strings give you a single authoritative source of valid permission identifiers. That fits well with the new controller/resource code that intersects or validates againstSubuser::allPermissionKeys().
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/Policies/Server/BackupPolicy.php (1)
17-20: Unused$recordparameters are expected; optionally silence static‑analysis noiseThe
$recordparameters inview()anddelete()are unused but are part of Laravel’s expected policy method signatures, so they should not be removed. If PHPMD/other tools are too noisy, you can optionally:
- Add a small comment or annotation indicating the parameter is required but unused, or
- Rename to
/** @noinspection PhpUnusedParameterInspection */or_recordif your tooling respects that style.Not strictly necessary, but may keep static‑analysis output cleaner.
Also applies to: 27-30
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
app/Filament/Server/Resources/Allocations/AllocationResource.php(5 hunks)app/Filament/Server/Resources/Backups/BackupResource.php(6 hunks)app/Filament/Server/Resources/Databases/DatabaseResource.php(3 hunks)app/Filament/Server/Resources/Users/UserResource.php(7 hunks)app/Policies/Server/ActivityLogPolicy.php(2 hunks)app/Policies/Server/AllocationPolicy.php(2 hunks)app/Policies/Server/BackupPolicy.php(2 hunks)app/Policies/Server/DatabasePolicy.php(2 hunks)app/Policies/Server/FilePolicy.php(2 hunks)app/Policies/Server/SchedulePolicy.php(2 hunks)app/Policies/Server/ServerPolicy.php(2 hunks)app/Policies/Server/UserPolicy.php(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/Filament/Server/Resources/Databases/DatabaseResource.php
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-02T19:55:23.924Z
Learnt from: rmartinoscar
Repo: pelican-dev/panel PR: 1837
File: app/Filament/Server/Resources/Schedules/ScheduleResource.php:337-338
Timestamp: 2025-12-02T19:55:23.924Z
Learning: In Filament resources, when ViewAction is hidden using `->hidden(fn ($record) => static::getEditAuthorizationResponse($record)->allowed())`, this is intentional UX design: the ViewAction (read-only) should be hidden when users have edit privileges, showing the EditAction instead. The ViewAction serves as a fallback for users who only have view permissions. This pattern is correct and should not be flagged as inverted logic.
Applied to files:
app/Policies/Server/AllocationPolicy.phpapp/Policies/Server/FilePolicy.phpapp/Policies/Server/UserPolicy.php
📚 Learning: 2025-08-12T17:33:57.388Z
Learnt from: lancepioch
Repo: pelican-dev/panel PR: 1595
File: app/Services/Servers/ServerCreationService.php:182-184
Timestamp: 2025-08-12T17:33:57.388Z
Learning: In the Pelican Panel codebase, when updating allocations to assign them to servers, the preferred pattern is to use whereNull('server_id'), lockForUpdate(), and direct property assignment with save() rather than mass update methods, to prevent race conditions and mass-assignment issues while ensuring model events fire properly.
Applied to files:
app/Filament/Server/Resources/Allocations/AllocationResource.php
🧬 Code graph analysis (7)
app/Policies/Server/ServerPolicy.php (1)
app/Models/Subuser.php (2)
Subuser(24-148)doesPermissionExist(140-147)
app/Policies/Server/FilePolicy.php (7)
app/Policies/Server/ActivityLogPolicy.php (1)
view(17-20)app/Policies/Server/AllocationPolicy.php (4)
view(17-20)create(22-25)edit(27-30)delete(32-35)app/Policies/Server/BackupPolicy.php (3)
view(17-20)create(22-25)delete(27-30)app/Policies/Server/DatabasePolicy.php (4)
view(17-20)create(22-25)edit(27-30)delete(32-35)app/Policies/Server/SchedulePolicy.php (4)
view(17-20)create(22-25)edit(27-30)delete(32-35)app/Policies/Server/UserPolicy.php (4)
view(17-20)create(22-25)edit(27-30)delete(32-35)app/Models/User.php (1)
User(95-493)
app/Policies/Server/UserPolicy.php (9)
app/Policies/Server/ActivityLogPolicy.php (1)
view(17-20)app/Policies/Server/AllocationPolicy.php (4)
view(17-20)create(22-25)edit(27-30)delete(32-35)app/Policies/Server/BackupPolicy.php (3)
view(17-20)create(22-25)delete(27-30)app/Policies/Server/DatabasePolicy.php (4)
view(17-20)create(22-25)edit(27-30)delete(32-35)app/Policies/Server/FilePolicy.php (4)
view(17-20)create(22-25)edit(27-30)delete(32-35)app/Policies/Server/SchedulePolicy.php (4)
view(17-20)create(22-25)edit(27-30)delete(32-35)app/Policies/Admin/DefaultPolicies.php (3)
view(15-18)create(20-23)delete(30-33)app/Models/User.php (1)
User(95-493)app/Policies/Admin/UserPolicy.php (1)
delete(22-25)
app/Policies/Server/SchedulePolicy.php (7)
app/Policies/Server/ActivityLogPolicy.php (1)
view(17-20)app/Policies/Server/AllocationPolicy.php (4)
view(17-20)create(22-25)edit(27-30)delete(32-35)app/Policies/Server/BackupPolicy.php (3)
view(17-20)create(22-25)delete(27-30)app/Policies/Server/DatabasePolicy.php (4)
view(17-20)create(22-25)edit(27-30)delete(32-35)app/Policies/Server/FilePolicy.php (4)
view(17-20)create(22-25)edit(27-30)delete(32-35)app/Policies/Server/UserPolicy.php (4)
view(17-20)create(22-25)edit(27-30)delete(32-35)app/Policies/Admin/DefaultPolicies.php (3)
view(15-18)create(20-23)delete(30-33)
app/Policies/Server/ActivityLogPolicy.php (7)
app/Policies/Server/AllocationPolicy.php (1)
view(17-20)app/Policies/Server/BackupPolicy.php (1)
view(17-20)app/Policies/Server/DatabasePolicy.php (1)
view(17-20)app/Policies/Server/FilePolicy.php (1)
view(17-20)app/Policies/Server/SchedulePolicy.php (1)
view(17-20)app/Policies/Server/UserPolicy.php (1)
view(17-20)app/Models/User.php (1)
User(95-493)
app/Filament/Server/Resources/Users/UserResource.php (2)
app/Models/Subuser.php (3)
Subuser(24-148)allPermissionData(97-128)user(91-94)app/Models/User.php (1)
can(362-371)
app/Filament/Server/Resources/Backups/BackupResource.php (2)
app/helpers.php (1)
user(127-130)app/Services/Subusers/SubuserCreationService.php (1)
handle(39-77)
🪛 PHPMD (2.15.0)
app/Policies/Server/AllocationPolicy.php
17-17: Avoid unused parameters such as '$record'. (undefined)
(UnusedFormalParameter)
27-27: Avoid unused parameters such as '$record'. (undefined)
(UnusedFormalParameter)
32-32: Avoid unused parameters such as '$record'. (undefined)
(UnusedFormalParameter)
app/Policies/Server/BackupPolicy.php
17-17: Avoid unused parameters such as '$record'. (undefined)
(UnusedFormalParameter)
27-27: Avoid unused parameters such as '$record'. (undefined)
(UnusedFormalParameter)
app/Policies/Server/FilePolicy.php
17-17: Avoid unused parameters such as '$record'. (undefined)
(UnusedFormalParameter)
27-27: Avoid unused parameters such as '$record'. (undefined)
(UnusedFormalParameter)
32-32: Avoid unused parameters such as '$record'. (undefined)
(UnusedFormalParameter)
app/Policies/Server/UserPolicy.php
17-17: Avoid unused parameters such as '$record'. (undefined)
(UnusedFormalParameter)
27-27: Avoid unused parameters such as '$record'. (undefined)
(UnusedFormalParameter)
32-32: Avoid unused parameters such as '$record'. (undefined)
(UnusedFormalParameter)
app/Policies/Server/DatabasePolicy.php
17-17: Avoid unused parameters such as '$record'. (undefined)
(UnusedFormalParameter)
27-27: Avoid unused parameters such as '$record'. (undefined)
(UnusedFormalParameter)
32-32: Avoid unused parameters such as '$record'. (undefined)
(UnusedFormalParameter)
app/Policies/Server/SchedulePolicy.php
17-17: Avoid unused parameters such as '$record'. (undefined)
(UnusedFormalParameter)
27-27: Avoid unused parameters such as '$record'. (undefined)
(UnusedFormalParameter)
32-32: Avoid unused parameters such as '$record'. (undefined)
(UnusedFormalParameter)
app/Policies/Server/ActivityLogPolicy.php
17-17: Avoid unused parameters such as '$model'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (14)
app/Filament/Server/Resources/Allocations/AllocationResource.php (1)
5-5: SubuserPermission-based authorization wiring looks correct and consistentThe switch from the old Permission model to
SubuserPermissionhere is coherent:
AllocationUpdateis used for both editing notes and changing the primary allocation.AllocationDeleteguards the detach action.AllocationCreateguards adding new allocations.- All checks correctly scope to the current
$servertenant and use null-safeuser()?->can(...), keeping guests/read-only users locked out.This preserves prior intent while aligning with the new enum-based permission system; no changes needed from this file.
Also applies to: 60-61, 72-73, 84-85, 104-105
app/Policies/Server/ServerPolicy.php (1)
6-6: LGTM! Clean migration to enum-based permission validation.The change correctly delegates permission existence checking to the
Subusermodel, which now handles both string-based andSubuserPermissionenum-based validation throughdoesPermissionExist(). The authorization logic remains intact while aligning with the new enum-based permission system.Also applies to: 21-21
app/Policies/Server/SchedulePolicy.php (1)
5-5: LGTM! Consistent migration to SubuserPermission enum.All permission checks have been correctly updated to use the
SubuserPermissionenum constants. The method signatures and authorization logic remain unchanged, ensuring consistency with the broader refactoring effort.Note: The PHPMD warnings about unused
$recordparameters are false positives—these parameters are required by Laravel's policy method signatures.Also applies to: 14-14, 19-19, 24-24, 29-29, 34-34
app/Policies/Server/AllocationPolicy.php (1)
5-5: LGTM! Consistent enum-based authorization.The migration from
Permissionconstants toSubuserPermissionenum values is correctly implemented across all policy methods. The authorization flow remains consistent with other server policies.Note: The PHPMD warnings about unused
$recordparameters are false positives—these are required by Laravel's policy interface.Also applies to: 14-14, 19-19, 24-24, 29-29, 34-34
app/Policies/Server/ActivityLogPolicy.php (1)
5-5: LGTM! Proper migration to enum-based permissions.Both
viewAny()andview()methods correctly useSubuserPermission::ActivityRead, maintaining the read-only nature of activity logs while aligning with the new permission system.Note: The PHPMD warning about the unused
$modelparameter is a false positive—it's required by Laravel's policy signature.Also applies to: 14-14, 19-19
app/Filament/Server/Resources/Users/UserResource.php (3)
5-5: LGTM! Enhanced permission filtering.The migration to
Subuser::allPermissionData()is correct and adds a beneficial check to filter out hidden permissions from the UI (line 75-76). This ensures only relevant permissions are displayed to users during subuser management.Also applies to: 9-9, 74-77
141-141: LGTM! Proper enum-based authorization.Authorization checks for user creation and editing correctly use
SubuserPermission::UserCreateandSubuserPermission::UserUpdaterespectively, maintaining proper access control while aligning with the new enum-based system.Also applies to: 227-227
150-150: LGTM! WebSocket permission auto-inclusion.Both create and edit actions correctly ensure
SubuserPermission::WebsocketConnectis included in the permission set. This is essential for subusers to maintain real-time server communication functionality.Also applies to: 281-281
app/Policies/Server/DatabasePolicy.php (1)
5-5: LGTM! Complete enum migration.All database policy methods correctly use the corresponding
SubuserPermissionenum constants (DatabaseRead,DatabaseCreate,DatabaseUpdate,DatabaseDelete). The authorization logic is properly maintained throughout the refactoring.Note: The PHPMD warnings about unused
$recordparameters are false positives—these parameters are required by Laravel's policy signatures.Also applies to: 14-14, 19-19, 24-24, 29-29, 34-34
app/Policies/Server/FilePolicy.php (1)
5-5: LGTM! Granular file permission migration.The migration correctly preserves the distinction between
FileRead(for listing files inviewAny()) andFileReadContent(for viewing file contents inview()), maintaining the appropriate permission granularity. All other file operations properly use their corresponding enum constants.Note: The PHPMD warnings about unused
$recordparameters are false positives—these are required by Laravel's policy interface.Also applies to: 14-14, 19-19, 24-24, 29-29, 34-34
app/Policies/Server/UserPolicy.php (1)
5-5: LGTM! Clean CRUD permission migration.All user management policy methods correctly use the corresponding
SubuserPermissionenum constants (UserRead,UserCreate,UserUpdate,UserDelete). The refactoring maintains consistent authorization patterns across the codebase.Note: The PHPMD warnings about unused
$recordparameters are false positives—these parameters are required by Laravel's policy method signatures.Also applies to: 14-14, 19-19, 24-24, 29-29, 34-34
app/Filament/Server/Resources/Backups/BackupResource.php (2)
5-8: Enum import aligns with refactorThe
App\Enums\SubuserPermissionimport cleanly replaces the oldPermissionmodel usage in this resource; no remaining Permission references here and the enum fits the new project‑wide pattern.
131-131: Permission mapping looks consistent; verify enum integration withcan()
BackupDeletefor rename/lock and for controlling theis_lockedflag,BackupDownloadfor download,BackupRestorefor restore, andBackupCreatefor the toolbar create action all mirror the previous Permission constants and preserve behavior.- The use of
user()?->can(SubuserPermission::..., $server)assumes your authorization layer accepts enum abilities (or converts them to their string values). If Gate/can()is still strictly string‑based, you may need to pass->valueinstead or ensure the enum is handled centrally.Functionally this looks correct for the refactor; just confirm the enum abilities are wired correctly in your auth layer/tests so these checks don’t silently fail.
Also applies to: 162-162, 171-171, 179-179, 261-261, 272-273
app/Policies/Server/BackupPolicy.php (1)
5-5: Policy permission checks correctly migrated toSubuserPermission
viewAny/view→BackupReadcreate→BackupCreatedelete→BackupDelete- All checks target
Filament::getTenant(), which is consistent with server‑scoped backup permissions.This is a straightforward, behavior‑preserving replacement of the old Permission constants. As with the resource, just ensure your gate/authorization layer supports passing
SubuserPermissioncases directly into$user->can(...)(or converts them to their backing values) so these policies are actually invoked as expected.Also applies to: 14-14, 19-19, 24-24, 29-29
|
branch: main: They both have every perms yet there's one more now and they are in completely random order (It would be much better if it was ordered like navigation [console, files, backups, schedules, users, databases, network, activity, startup, settings]). |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/Filament/Server/Resources/Subusers/SubuserResource.php (2)
68-101: Permission tabs: consider stabilizing ordering and hardeninghiddenhandlingThe refactor to
Subuser::allPermissionData()and skipping hidden groups looks good and matches the new enum-based permission API. However:
- The tab / checkbox ordering now depends entirely on the order returned by
allPermissionData(), which is what surfaced as “random” ordering in the UI discussion. If you want to keep a consistent order (e.g. console, files, backups, schedules, users, databases, network, activity, startup, settings), consider either:
- enforcing that order inside
Subuser::allPermissionData(), or- sorting the returned array here before the
foreach, with any unknown/custom groups appended to the end.- For third‑party/custom permission groups, a missing
hiddenkey would currently trigger a PHP notice. If the intent is to treat missing asfalse, you can make this slightly more defensive:-foreach (Subuser::allPermissionData() as $data) { - if ($data['hidden']) { +foreach (Subuser::allPermissionData() as $data) { + if ($data['hidden'] ?? false) { continue; }These are UX/robustness tweaks; the current logic is otherwise sound.
144-146: Optional: GuardWebsocketConnectpermission with existence check for defensive consistencyBoth update and invite flows always add
SubuserPermission::WebsocketConnect->valuebefore de-duplicating permissions (lines 144-146 and 271-276):->push(SubuserPermission::WebsocketConnect->value) ->unique() ->all();This ensures the websocket permission is always present. To make the behavior more resilient to future permission customizations, consider guarding with
Subuser::doesPermissionExist()(a method already used elsewhere in the codebase likeServerPolicy.php):- ->push(SubuserPermission::WebsocketConnect->value) + ->when( + Subuser::doesPermissionExist(SubuserPermission::WebsocketConnect), + fn ($permissions) => $permissions->push(SubuserPermission::WebsocketConnect->value), + ) ->unique() ->all();This prevents passing invalid permissions downstream if permission sets are customized more aggressively in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
app/Filament/Server/Resources/Subusers/SubuserResource.php(6 hunks)app/Policies/ActivityLogPolicy.php(2 hunks)app/Policies/AllocationPolicy.php(2 hunks)app/Policies/BackupPolicy.php(2 hunks)app/Policies/DatabasePolicy.php(2 hunks)app/Policies/FilePolicy.php(2 hunks)app/Policies/SchedulePolicy.php(2 hunks)app/Policies/ServerPolicy.php(2 hunks)app/Policies/SubuserPolicy.php(2 hunks)tests/Feature/SettingsControllerTest.php(10 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-06T10:11:54.301Z
Learnt from: Boy132
Repo: pelican-dev/panel PR: 1866
File: app/Policies/PluginPolicy.php:7-7
Timestamp: 2025-11-06T10:11:54.301Z
Learning: In the codebase, the `DefaultPolicies` trait is located in the `App\Policies` namespace (file: app/Policies/DefaultPolicies.php). Policy classes in the same namespace can use this trait without requiring an import statement.
Applied to files:
app/Policies/FilePolicy.phpapp/Policies/SubuserPolicy.phpapp/Policies/BackupPolicy.phpapp/Policies/ActivityLogPolicy.phpapp/Policies/AllocationPolicy.phpapp/Policies/SchedulePolicy.phpapp/Policies/DatabasePolicy.php
📚 Learning: 2025-12-02T19:55:23.957Z
Learnt from: rmartinoscar
Repo: pelican-dev/panel PR: 1837
File: app/Filament/Server/Resources/Schedules/ScheduleResource.php:337-338
Timestamp: 2025-12-02T19:55:23.957Z
Learning: In Filament resources, when ViewAction is hidden using `->hidden(fn ($record) => static::getEditAuthorizationResponse($record)->allowed())`, this is intentional UX design: the ViewAction (read-only) should be hidden when users have edit privileges, showing the EditAction instead. The ViewAction serves as a fallback for users who only have view permissions. This pattern is correct and should not be flagged as inverted logic.
Applied to files:
app/Policies/FilePolicy.phpapp/Policies/SubuserPolicy.php
🧬 Code graph analysis (6)
app/Policies/FilePolicy.php (6)
app/Policies/ActivityLogPolicy.php (1)
view(17-20)app/Policies/AllocationPolicy.php (4)
view(17-20)create(22-25)edit(27-30)delete(32-35)app/Policies/DatabasePolicy.php (4)
view(17-20)create(22-25)edit(27-30)delete(32-35)app/Policies/SchedulePolicy.php (4)
view(17-20)create(22-25)edit(27-30)delete(32-35)app/Policies/SubuserPolicy.php (4)
view(17-20)create(22-25)edit(27-30)delete(32-35)app/Models/User.php (1)
User(95-493)
app/Policies/SubuserPolicy.php (9)
app/Policies/ActivityLogPolicy.php (1)
view(17-20)app/Policies/AllocationPolicy.php (4)
view(17-20)create(22-25)edit(27-30)delete(32-35)app/Policies/BackupPolicy.php (3)
view(17-20)create(22-25)delete(27-30)app/Policies/DatabasePolicy.php (4)
view(17-20)create(22-25)edit(27-30)delete(32-35)app/Policies/FilePolicy.php (4)
view(17-20)create(22-25)edit(27-30)delete(32-35)app/Policies/SchedulePolicy.php (4)
view(17-20)create(22-25)edit(27-30)delete(32-35)app/Policies/DefaultAdminPolicies.php (3)
view(15-18)create(20-23)delete(30-33)app/Models/User.php (1)
User(95-493)app/Policies/UserPolicy.php (1)
delete(22-25)
app/Policies/ActivityLogPolicy.php (7)
app/Policies/AllocationPolicy.php (1)
view(17-20)app/Policies/BackupPolicy.php (1)
view(17-20)app/Policies/DatabasePolicy.php (1)
view(17-20)app/Policies/FilePolicy.php (1)
view(17-20)app/Policies/SchedulePolicy.php (1)
view(17-20)app/Policies/SubuserPolicy.php (1)
view(17-20)app/Models/User.php (1)
User(95-493)
app/Policies/ServerPolicy.php (1)
app/Models/Subuser.php (2)
Subuser(24-148)doesPermissionExist(140-147)
app/Policies/SchedulePolicy.php (3)
app/Policies/ActivityLogPolicy.php (1)
view(17-20)app/Policies/BackupPolicy.php (3)
view(17-20)create(22-25)delete(27-30)app/Policies/SubuserPolicy.php (4)
view(17-20)create(22-25)edit(27-30)delete(32-35)
app/Filament/Server/Resources/Subusers/SubuserResource.php (2)
app/Models/Subuser.php (2)
Subuser(24-148)allPermissionData(97-128)app/Http/Controllers/Api/Client/ClientController.php (1)
permissions(85-93)
🪛 PHPMD (2.15.0)
app/Policies/FilePolicy.php
17-17: Avoid unused parameters such as '$record'. (undefined)
(UnusedFormalParameter)
27-27: Avoid unused parameters such as '$record'. (undefined)
(UnusedFormalParameter)
32-32: Avoid unused parameters such as '$record'. (undefined)
(UnusedFormalParameter)
app/Policies/SubuserPolicy.php
17-17: Avoid unused parameters such as '$record'. (undefined)
(UnusedFormalParameter)
27-27: Avoid unused parameters such as '$record'. (undefined)
(UnusedFormalParameter)
32-32: Avoid unused parameters such as '$record'. (undefined)
(UnusedFormalParameter)
app/Policies/BackupPolicy.php
17-17: Avoid unused parameters such as '$record'. (undefined)
(UnusedFormalParameter)
27-27: Avoid unused parameters such as '$record'. (undefined)
(UnusedFormalParameter)
app/Policies/ActivityLogPolicy.php
17-17: Avoid unused parameters such as '$model'. (undefined)
(UnusedFormalParameter)
app/Policies/AllocationPolicy.php
17-17: Avoid unused parameters such as '$record'. (undefined)
(UnusedFormalParameter)
27-27: Avoid unused parameters such as '$record'. (undefined)
(UnusedFormalParameter)
32-32: Avoid unused parameters such as '$record'. (undefined)
(UnusedFormalParameter)
app/Policies/SchedulePolicy.php
17-17: Avoid unused parameters such as '$record'. (undefined)
(UnusedFormalParameter)
27-27: Avoid unused parameters such as '$record'. (undefined)
(UnusedFormalParameter)
32-32: Avoid unused parameters such as '$record'. (undefined)
(UnusedFormalParameter)
app/Policies/DatabasePolicy.php
17-17: Avoid unused parameters such as '$record'. (undefined)
(UnusedFormalParameter)
27-27: Avoid unused parameters such as '$record'. (undefined)
(UnusedFormalParameter)
32-32: Avoid unused parameters such as '$record'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (12)
app/Policies/ActivityLogPolicy.php (1)
5-5: LGTM! Clean migration to SubuserPermission enum.The refactoring correctly updates the permission checks to use the new
SubuserPermission::ActivityReadenum value, improving type safety over the previous string-based constants.Also applies to: 14-14, 19-19
app/Policies/BackupPolicy.php (1)
5-5: LGTM! Consistent permission enum migration.All backup-related permission checks have been correctly migrated to use the
SubuserPermissionenum values (BackupRead, BackupCreate, BackupDelete), maintaining the same authorization logic.Also applies to: 14-14, 19-19, 24-24, 29-29
app/Policies/SubuserPolicy.php (1)
5-5: LGTM! Complete CRUD permission coverage.All subuser management operations are properly covered with the corresponding
SubuserPermissionenum values (UserRead, UserCreate, UserUpdate, UserDelete).Also applies to: 14-14, 19-19, 24-24, 29-29, 34-34
app/Policies/ServerPolicy.php (1)
6-6: LGTM! Proper adaptation of generic permission validation.The change to use
Subuser::doesPermissionExist($ability)is appropriate for thebefore()hook, which needs to validate whether a dynamic ability string represents a valid subuser permission before evaluating access rules.Also applies to: 25-25
app/Policies/DatabasePolicy.php (1)
5-5: LGTM! Complete database permission coverage.All database operations are correctly mapped to their corresponding
SubuserPermissionenum values (DatabaseRead, DatabaseCreate, DatabaseUpdate, DatabaseDelete).Also applies to: 14-14, 19-19, 24-24, 29-29, 34-34
app/Policies/SchedulePolicy.php (1)
5-5: LGTM! Schedule permissions properly migrated.All schedule-related operations are correctly updated to use the
SubuserPermissionenum values (ScheduleRead, ScheduleCreate, ScheduleUpdate, ScheduleDelete).Also applies to: 14-14, 19-19, 24-24, 29-29, 34-34
app/Policies/FilePolicy.php (1)
5-5: LGTM! File permissions with proper granularity.The migration correctly maintains the granular permission model for file operations, distinguishing between
FileRead(listing files) andFileReadContent(viewing file contents), along with the standard CRUD operations.Also applies to: 14-14, 19-19, 24-24, 29-29, 34-34
app/Policies/AllocationPolicy.php (1)
5-5: LGTM! Allocation permissions complete.All network allocation operations are correctly migrated to use the
SubuserPermissionenum values (AllocationRead, AllocationCreate, AllocationUpdate, AllocationDelete), completing the consistent refactoring across all policy files.Also applies to: 14-14, 19-19, 24-24, 29-29, 34-34
tests/Feature/SettingsControllerTest.php (3)
4-179: Enum permission migration in tests looks consistentThe switch from the old Permission model constants to
SubuserPermission(import at Line 4 and allgenerateTestAccount([...])calls) keeps the same permission semantics per test:
- rename tests use
WebsocketConnect(for the “cannot rename” case) andWebsocketConnect + SettingsRename(for the “can rename” case),- description tests use
SettingsDescription,- docker image tests use
WebsocketConnectfor the unauthorized case andStartupDockerImagefor the authorized cases,- reinstall uses
SettingsReinstall.Given
generateTestAccountis documented to accept permission enums/values directly, this is a straightforward, type‑safer refactor with no behavior change in the test flows. Based on relevant snippet intest/Pest.php, this looks correct.
13-76: Confirm intended base-permission requirements for rename vs descriptionRename tests now explicitly require
WebsocketConnect+SettingsRenamefor the positive case (Line 64), while description tests only requireSettingsDescription(Lines 29, 48). If the new permission system is meant to standardize on a common “base” permission (e.g.,WebsocketConnector similar) for all settings mutations, it might be worth double‑checking that this asymmetry between rename and description is intentional and reflected in the policies, not just legacy behavior preserved from the oldPermissionmodel.
78-179: Verify docker-image and reinstall tests match updated policy expectationsFor docker image and reinstall endpoints, the tests now use:
WebsocketConnectonly for the “unauthorized to change docker image” case (Line 79),StartupDockerImagefor all “can/cannot change docker image” cases (Lines 95, 115, 138),SettingsReinstallfor the reinstall case (Line 158).This mirrors the old Permission‑constant usage, but it’s worth confirming that, under the refactored
Subuserpermission logic, these endpoints don’t also require some additional base permission (likeWebsocketConnect) for authorized calls, and that the policy layer is aligned with what these tests encode.app/Filament/Server/Resources/Subusers/SubuserResource.php (1)
5-5: Enum-based authorization implementation is correctly aligned across all layersThe SubuserPermission enum is properly imported and used consistently:
- Edit action (line 137):
user()?->can(SubuserPermission::UserUpdate, $server)✓- Invite/Create action (line 220):
user()?->can(SubuserPermission::UserCreate, $server)✓- SubuserPolicy methods match exactly:
create()usesUserCreate,edit()usesUserUpdate- All authorization checks use the same enum values throughout the codebase
Policy/gate definitions are properly aligned with the enum, ensuring no drift between what's checked in actions and what's defined in policies.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/Enums/SubuserPermission.php (2)
7-58: Consider reordering enum cases to match desired UI grouping/orderEnum case order will typically dictate how permissions appear when iterated (e.g., in the UI). Right now the groups are ordered as: websocket, control, file, backup, schedule, user, database, allocation, activity, startup, settings. The PR discussion suggests a more intentional order like: console(control), files(file), backups(backup), schedules(schedule), users(user), databases(database), network(allocation), activity(activity), startup(startup), settings(settings).
If the UI builds from
SubuserPermissiondirectly (or viaSubuser::allPermissionKeys()in declaration order), it would be worth reordering the enum cases to match that sequence so the display feels consistent and non-random.
65-68: ExplicitisHidden()behavior looks fine but may need future-proofingHard-coding
WebsocketConnectas the only hidden permission is clear and easy to follow. If more “hidden-only” permissions are added in the future, consider switching this to amatchor a smallin_array([...])to keep the logic from growing into a long chain of comparisons.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/Enums/SubuserPermission.php(1 hunks)
🧰 Additional context used
🪛 PHPMD (2.15.0)
app/Enums/SubuserPermission.php
72-72: Avoid unused local variables such as '$permission'. (undefined)
(UnusedLocalVariable)
🔇 Additional comments (1)
app/Enums/SubuserPermission.php (1)
59-64:split()implementation is straightforward and clear
split()correctly returns[group, permission]by splitting on the first dot with a limit of 2, which matches how the enum values are structured. The docblock return type also looks good.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/Filament/Server/Resources/Subusers/SubuserResource.php (1)
71-75: Hidden groups handled correctly; consider enforcing a stable permission orderSkipping entries with
$data['hidden']and building tabs/options fromSubuser::allPermissionData()looks good, and keeping hidden groups out of$permissionsArrayensures “Assign all” doesn’t touch system/hidden permissions.Given earlier feedback about “random” permission ordering in the UI, the order here is entirely dictated by the order returned from
Subuser::allPermissionData()(and by thepermissionsarrays within each group). If that method aggregates from multiple sources or enums without explicit sorting, you’ll keep seeing ordering changes across refactors. Consider:
- Defining a canonical group order (e.g.
['console', 'files', 'backups', 'schedules', 'users', 'databases', 'network', 'activity', 'startup', 'settings']) and sorting the data against it, ideally insideSubuser::allPermissionData(), or- Applying the same ordering just before this
foreachso the Filament resource doesn’t depend on incidental array order.This would directly address the “random order” concern while keeping the hidden‑group behavior as-is.
Also applies to: 85-101
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/Filament/Server/Resources/Subusers/SubuserResource.php(7 hunks)
🔇 Additional comments (3)
app/Filament/Server/Resources/Subusers/SubuserResource.php (3)
5-5: Enum import aligns with new permission modelSwitching to
App\Enums\SubuserPermissionhere is consistent with the PR’s move away from thePermissionmodel; no issues spotted.
137-137: Enum-based authorization for edit/invite looks consistentUsing
user()?->can(SubuserPermission::UserUpdate, $server)for the edit action andSubuserPermission::UserCreatefor the invite action matches the new enum-driven permission model and clearly separates “edit” vs “invite” capabilities. Assuming corresponding policies/gate checks already accept these enum cases, this is a clean, type-safe replacement for the old constants.Also applies to: 220-220
144-144: Always addingWebsocketConnectcentralizes the hidden/system permissionAppending
SubuserPermission::WebsocketConnect->valueand then->unique()in both the update and invite flows ensures every subuser retains this hidden/system permission regardless of the checkbox selections, which aligns with the hidden-group handling in the form.This looks intentional and matches the idea of a non-user-configurable permission. If that’s the desired behavior (i.e., subusers should never lose websocket access), the implementation here is sound.
Also applies to: 274-274
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/Filament/Server/Resources/Subusers/SubuserResource.php (1)
71-74: Consider ordering permissions consistently for better UX.As noted in the PR comments, permissions currently appear in the order returned by
Subuser::allPermissionData(), which may not be predictable. For a more consistent user experience, consider sorting the permission groups before building the tabs.Apply this diff to sort permissions in a consistent order:
- foreach (Subuser::allPermissionData() as $data) { + $desiredOrder = ['console', 'files', 'backups', 'schedules', 'users', 'databases', 'network', 'activity', 'startup', 'settings']; + $permissionData = collect(Subuser::allPermissionData()) + ->sortBy(fn ($data) => array_search($data['name'], $desiredOrder, true) !== false + ? array_search($data['name'], $desiredOrder, true) + : PHP_INT_MAX); + + foreach ($permissionData as $data) { if ($data['hidden']) { continue; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/Filament/Server/Resources/Subusers/SubuserResource.php(7 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-02T19:55:23.957Z
Learnt from: rmartinoscar
Repo: pelican-dev/panel PR: 1837
File: app/Filament/Server/Resources/Schedules/ScheduleResource.php:337-338
Timestamp: 2025-12-02T19:55:23.957Z
Learning: In Filament resources, when ViewAction is hidden using `->hidden(fn ($record) => static::getEditAuthorizationResponse($record)->allowed())`, this is intentional UX design: the ViewAction (read-only) should be hidden when users have edit privileges, showing the EditAction instead. The ViewAction serves as a fallback for users who only have view permissions. This pattern is correct and should not be flagged as inverted logic.
Applied to files:
app/Filament/Server/Resources/Subusers/SubuserResource.php
🧬 Code graph analysis (1)
app/Filament/Server/Resources/Subusers/SubuserResource.php (2)
app/Models/Subuser.php (2)
Subuser(24-148)allPermissionData(97-128)app/Enums/SubuserPermission.php (1)
isHidden(65-68)
🔇 Additional comments (5)
app/Filament/Server/Resources/Subusers/SubuserResource.php (5)
5-5: LGTM!The import of
SubuserPermissionenum correctly supports the refactoring from the Permission model.
71-74: LGTM!The filtering logic correctly excludes hidden permission groups from the UI tabs. The use of
Subuser::allPermissionData()properly centralizes permission metadata.
91-91: LGTM!The
->contained(false)styling adjustment appears intentional based on the Section containment control changes described in the PR.
117-122: LGTM!The permissions count calculation correctly filters out hidden permissions while properly handling custom permissions. The use of
tryFrom()with the?? falsefallback ensures custom permissions (not in the enum) are counted, which is the desired behavior.This implementation addresses the previous review concern about the brittle magic
-1subtraction.
140-140: LGTM!The authorization checks and permission assignments correctly use the
SubuserPermissionenum:
- Authorization checks use
SubuserPermission::UserUpdateandSubuserPermission::UserCreate- WebsocketConnect permission is properly pushed using
SubuserPermission::WebsocketConnect->valuein both create and update flowsThis refactoring eliminates hard-coded permission strings and leverages type-safe enum values.
Also applies to: 147-147, 223-223, 277-277
(Backend) Refactors for subuser permissions:
PermissionsmodelSubusermodelPermission names have not changed! So (in theory) no breaking changes for existing installs!