-
-
Notifications
You must be signed in to change notification settings - Fork 254
Fix permission checks on Client side #1913
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
📝 WalkthroughWalkthroughTwo authorization checks were changed: the Settings page description field and its save flow now use ACTION_SETTINGS_DESCRIPTION instead of ACTION_SETTINGS_RENAME; the ScheduleTaskController delete action now requires ACTION_SCHEDULE_DELETE instead of ACTION_SCHEDULE_UPDATE. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as Settings Page
participant Auth as AuthorizationService
participant Server
User->>UI: open Settings
UI->>Auth: check ACTION_SETTINGS_DESCRIPTION
alt permitted
UI-->>User: description editable
User->>UI: submit description update
UI->>Auth: authorize update (ACTION_SETTINGS_DESCRIPTION)
Auth-->>UI: allow
UI->>Server: save description
Server-->>UI: success
else forbidden
UI-->>User: field disabled / save blocked
end
sequenceDiagram
participant Client
participant API as ScheduleTaskController
participant Auth as AuthorizationService
participant Server
Client->>API: DELETE /servers/:id/schedules/:task
API->>Auth: check ACTION_SCHEDULE_DELETE for server
alt permitted
API->>Server: delete scheduled task
Server-->>API: success
API-->>Client: 204 No Content
else forbidden
API-->>Client: 403 Forbidden
end
Pre-merge checks❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)app/Filament/Server/Pages/Settings.php (3)
🔇 Additional comments (2)
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/Filament/Server/Pages/Settings.php(1 hunks)app/Http/Controllers/Api/Client/Servers/ScheduleTaskController.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
app/Http/Controllers/Api/Client/Servers/ScheduleTaskController.php (1)
app/Models/Permission.php (1)
Permission(11-221)
app/Filament/Server/Pages/Settings.php (3)
app/helpers.php (1)
user(127-130)app/Models/User.php (1)
can(356-369)app/Models/Permission.php (1)
Permission(11-221)
🔇 Additional comments (1)
app/Http/Controllers/Api/Client/Servers/ScheduleTaskController.php (1)
167-175: Verified: UsingACTION_SCHEDULE_DELETEfor task deletion is the correct permission.The Permission model confirms there is no separate
ACTION_TASK_DELETEconstant—tasks are managed under the schedule permission hierarchy. UsingACTION_SCHEDULE_DELETEappropriately gates this destructive operation at the schedule level and is consistent with the permission model design (schedule.read/create/update/delete). The change correctly tightens authorization semantics.
No description provided.