Use Policies rather then overriding can*() functions
#1837
Conversation
📝 WalkthroughWalkthroughAuthorization was moved from Filament Resource Changes
Sequence Diagram(s)sequenceDiagram
participant Resource as Filament Resource
participant Gate as Laravel Gate
participant Discovery as Gate::guessPolicyNamesUsing
participant Policy as Policy Class
participant User as Authenticated User
Resource->>Gate: authorize(action, model)
Gate->>Discovery: guess policy for model
Discovery->>Discovery: get Filament panel id\nconstruct App\Policies\{Panel}\{Model}Policy
Discovery-->>Gate: policy class or null
Gate->>Policy: call policy method (viewAny/view/create/edit/delete)
Policy->>User: user()?->can(ACTION_*, Filament::getTenant())
User-->>Policy: permission bool
Policy-->>Gate: authorization result
Gate-->>Resource: allow / deny
sequenceDiagram
participant Old as Old (Resource can*() overrides)
participant New as New (Policies + Gate discovery)
rect rgb(230,230,255)
Old->>Old: Resource::canViewAny()/canCreate()/canEdit()/canDelete()
Old-->>Old: return boolean
end
rect rgb(230,255,230)
New->>Gate: Resource requests authorization
Gate->>Policy: Policy::viewAny()/create()/edit()/delete()
Policy->>User: user()?->can(ACTION_*, tenant)
Policy-->>Gate: AuthorizationResponse / bool
end
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (14)
app/Policies/Server/ServerPolicies.php (1)
9-14: Consider adding explicit policy methods for standard CRUD operations.While the
before()method handles permission-based authorization, the policy relies on theDefaultPoliciestrait for standard CRUD operations (viewAny,view,create,update,delete). According to the PR objectives, explicit policy methods with response objects are preferred over relying on fallback behavior.Consider adding explicit methods like:
public function viewAny(User $user): bool { return true; // or your specific logic } public function view(User $user, Server $server): bool { return true; // handled by before() in most cases } public function create(User $user): bool { // Define explicit create authorization return $user->canCreateServers(); } // Additional methods as neededThis would make the authorization behavior more explicit and align with the broader refactoring goals.
app/Policies/Admin/MountPolicy.php (1)
14-28: Harden before() signature to avoid arg count/type pitfalls.Gate may call before($user, $ability, ...$args) with zero or a class-string. A required third param can trigger an ArgumentCountError. Make it nullable with a default and guard type.
Apply:
- public function before(User $user, string $ability, string|Mount $mount): ?bool + public function before(User $user, string $ability, string|Mount|null $mount = null): ?bool { - // For "viewAny" the $mount param is the class name - if (is_string($mount)) { + // For "viewAny" the $mount param is the class name; some calls may pass no subject. + if (! $mount instanceof Mount) { return null; } - foreach ($mount->nodes as $node) { + foreach ($mount->nodes as $node) { if (!$user->canTarget($node)) { return false; } } return null; }Optional:
$mount->loadMissing('nodes');before the loop to reduce repeated lazy load.app/Policies/Admin/DefaultPolicies.php (1)
10-44: Consider returning policy Response objects for richer feedback.Current bool returns work, but swapping to Response enables messages/reasons and aligns with the PR’s “policy response objects” goal.
Example:
+use Illuminate\Auth\Access\Response; @@ - public function viewAny(User $user): bool + public function viewAny(User $user): bool|\Illuminate\Auth\Access\Response { - return $user->can('viewList ' . $this->modelName); + return $user->can('viewList ' . $this->modelName) + ? Response::allow() + : Response::deny(); }Repeat similarly for view/create/update/delete/replicate if you want consistent messaging. Otherwise, leaving bools is fine.
app/Filament/Server/Resources/Schedules/ScheduleResource.php (1)
338-339: Use getEditAuthorizationResponse() instead of canEdit() for consistency with policy responses.Aligns the UI logic with the new authorization flow and avoids the legacy can* API.
Apply:
- ViewAction::make() - ->hidden(fn ($record) => static::canEdit($record)), + ViewAction::make() + ->hidden(fn (Schedule $record) => static::getEditAuthorizationResponse($record)->allowed()),app/Policies/Server/UserPolicy.php (3)
23-26: Silence PHPMD UnusedFormalParameter for $record or consume itEither suppress or mark it consumed; keeps signatures intact for future use.
Option A — suppression:
public static function edit(Model $record): bool { + /** @noinspection PhpUnusedParameterInspection */ + /** @SuppressWarnings(PHPMD.UnusedFormalParameter) */ return user()?->can(Permission::ACTION_USER_UPDATE, Filament::getTenant()) ?? false; } @@ public static function delete(Model $record): bool { + /** @noinspection PhpUnusedParameterInspection */ + /** @SuppressWarnings(PHPMD.UnusedFormalParameter) */ return user()?->can(Permission::ACTION_USER_DELETE, Filament::getTenant()) ?? false; }Option B — explicitly consume:
- public static function edit(Model $record): bool + public static function edit(Model $record): bool { + // parameter currently unused but kept for signature parity + unset($record); return user()?->can(Permission::ACTION_USER_UPDATE, Filament::getTenant()) ?? false; }Also applies to: 28-31
23-31: Provide update() alias for broader compatibilityMany callers/Gate conventions use update(). Add a thin alias calling edit().
public static function edit(Model $record): bool { return user()?->can(Permission::ACTION_USER_UPDATE, Filament::getTenant()) ?? false; } + + public static function update(Model $record): bool + { + return self::edit($record); + }
13-31: Consider returning Laravel policy Responses to carry reasonsIf PR goal targets richer responses, these methods can return Response::allow()/deny() instead of bools, or let Resource get*AuthorizationResponse() wrap reasons. Confirm intended design.
app/Policies/Server/SchedulePolicy.php (2)
23-26: Suppress or consume $record to appease PHPMDKeep signature parity; silence the warning or unset the param.
public static function edit(Model $record): bool { + /** @noinspection PhpUnusedParameterInspection */ + /** @SuppressWarnings(PHPMD.UnusedFormalParameter) */ return user()?->can(Permission::ACTION_SCHEDULE_UPDATE, Filament::getTenant()) ?? false; } @@ public static function delete(Model $record): bool { + /** @noinspection PhpUnusedParameterInspection */ + /** @SuppressWarnings(PHPMD.UnusedFormalParameter) */ return user()?->can(Permission::ACTION_SCHEDULE_DELETE, Filament::getTenant()) ?? false; }Also applies to: 28-31
23-31: Add update() alias and optional view() for parity
- Provide update() alias for compatibility.
- Optionally add view() mirroring read permission to match DatabasePolicy.
public static function edit(Model $record): bool { return user()?->can(Permission::ACTION_SCHEDULE_UPDATE, Filament::getTenant()) ?? false; } + + public static function update(Model $record): bool + { + return self::edit($record); + } + + public static function view(Model $record): bool + { + return user()?->can(Permission::ACTION_SCHEDULE_READ, Filament::getTenant()) ?? false; + }app/Policies/Server/AllocationPolicy.php (2)
23-26: Suppress or consume $record parameterSilence PHPMD while preserving signatures.
public static function edit(Model $record): bool { + /** @noinspection PhpUnusedParameterInspection */ + /** @SuppressWarnings(PHPMD.UnusedFormalParameter) */ return user()?->can(Permission::ACTION_ALLOCATION_UPDATE, Filament::getTenant()) ?? false; } @@ public static function delete(Model $record): bool { + /** @noinspection PhpUnusedParameterInspection */ + /** @SuppressWarnings(PHPMD.UnusedFormalParameter) */ return user()?->can(Permission::ACTION_ALLOCATION_DELETE, Filament::getTenant()) ?? false; }Also applies to: 28-31
23-31: Add update() alias and optional view()Improves compatibility and parity across policies.
public static function edit(Model $record): bool { return user()?->can(Permission::ACTION_ALLOCATION_UPDATE, Filament::getTenant()) ?? false; } + + public static function update(Model $record): bool + { + return self::edit($record); + } + + public static function view(Model $record): bool + { + return user()?->can(Permission::ACTION_ALLOCATION_READ, Filament::getTenant()) ?? false; + }app/Policies/Server/DatabasePolicy.php (3)
18-21: Suppress or consume $record to resolve PHPMD warningsSame approach as others to avoid UnusedFormalParameter.
public static function view(Model $record): bool { + /** @noinspection PhpUnusedParameterInspection */ + /** @SuppressWarnings(PHPMD.UnusedFormalParameter) */ return user()?->can(Permission::ACTION_DATABASE_READ, Filament::getTenant()) ?? false; } @@ public static function edit(Model $record): bool { + /** @noinspection PhpUnusedParameterInspection */ + /** @SuppressWarnings(PHPMD.UnusedFormalParameter) */ return user()?->can(Permission::ACTION_DATABASE_UPDATE, Filament::getTenant()) ?? false; } @@ public static function delete(Model $record): bool { + /** @noinspection PhpUnusedParameterInspection */ + /** @SuppressWarnings(PHPMD.UnusedFormalParameter) */ return user()?->can(Permission::ACTION_DATABASE_DELETE, Filament::getTenant()) ?? false; }Also applies to: 28-31, 33-36
28-31: Add update() alias to complement edit()Aligns with common ability naming.
public static function edit(Model $record): bool { return user()?->can(Permission::ACTION_DATABASE_UPDATE, Filament::getTenant()) ?? false; } + + public static function update(Model $record): bool + { + return self::edit($record); + }
11-11: Optional: constant instead of mutable propertyIf $modelName is constant metadata, prefer a class constant for clarity and immutability.
- protected string $modelName = 'database'; + public const MODEL_NAME = 'database';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
app/Filament/Server/Resources/Activities/ActivityResource.php(0 hunks)app/Filament/Server/Resources/Allocations/AllocationResource.php(0 hunks)app/Filament/Server/Resources/Backups/BackupResource.php(0 hunks)app/Filament/Server/Resources/Databases/DatabaseResource.php(0 hunks)app/Filament/Server/Resources/Files/FileResource.php(0 hunks)app/Filament/Server/Resources/Schedules/ScheduleResource.php(1 hunks)app/Filament/Server/Resources/Users/UserResource.php(0 hunks)app/Policies/Admin/ApiKeyPolicy.php(1 hunks)app/Policies/Admin/DatabaseHostPolicy.php(1 hunks)app/Policies/Admin/DefaultPolicies.php(1 hunks)app/Policies/Admin/EggPolicy.php(1 hunks)app/Policies/Admin/MountPolicy.php(1 hunks)app/Policies/Admin/NodePolicy.php(1 hunks)app/Policies/Admin/RolePolicy.php(1 hunks)app/Policies/Admin/UserPolicy.php(1 hunks)app/Policies/Admin/WebhookConfigurationPolicy.php(1 hunks)app/Policies/DatabasePolicy.php(0 hunks)app/Policies/Server/ActivityLogPolicy.php(1 hunks)app/Policies/Server/AllocationPolicy.php(1 hunks)app/Policies/Server/BackupPolicy.php(1 hunks)app/Policies/Server/DatabasePolicy.php(1 hunks)app/Policies/Server/DefaultPolicies.php(1 hunks)app/Policies/Server/FilePolicy.php(1 hunks)app/Policies/Server/SchedulePolicy.php(1 hunks)app/Policies/Server/ServerPolicies.php(1 hunks)app/Policies/Server/UserPolicy.php(1 hunks)app/Providers/AppServiceProvider.php(2 hunks)
💤 Files with no reviewable changes (7)
- app/Filament/Server/Resources/Activities/ActivityResource.php
- app/Filament/Server/Resources/Files/FileResource.php
- app/Policies/DatabasePolicy.php
- app/Filament/Server/Resources/Users/UserResource.php
- app/Filament/Server/Resources/Backups/BackupResource.php
- app/Filament/Server/Resources/Databases/DatabaseResource.php
- app/Filament/Server/Resources/Allocations/AllocationResource.php
🧰 Additional context used
🧬 Code graph analysis (8)
app/Policies/Server/DatabasePolicy.php (3)
app/Models/Permission.php (1)
Permission(11-221)app/Models/Database.php (1)
Database(29-195)app/Policies/Server/AllocationPolicy.php (4)
viewAny(13-16)create(18-21)edit(23-26)delete(28-31)
app/Policies/Server/SchedulePolicy.php (4)
app/Models/Permission.php (1)
Permission(11-221)app/Policies/Server/ActivityLogPolicy.php (1)
viewAny(12-15)app/Policies/Server/AllocationPolicy.php (4)
viewAny(13-16)create(18-21)edit(23-26)delete(28-31)app/Policies/Server/DatabasePolicy.php (4)
viewAny(13-16)create(23-26)edit(28-31)delete(33-36)
app/Policies/Server/BackupPolicy.php (3)
app/Models/Permission.php (1)
Permission(11-221)app/Policies/Server/ActivityLogPolicy.php (1)
viewAny(12-15)app/Policies/Server/AllocationPolicy.php (3)
viewAny(13-16)create(18-21)delete(28-31)
app/Policies/Server/ActivityLogPolicy.php (3)
app/Models/Permission.php (1)
Permission(11-221)app/Policies/Server/AllocationPolicy.php (1)
viewAny(13-16)app/Policies/Server/DatabasePolicy.php (2)
viewAny(13-16)view(18-21)
app/Policies/Server/UserPolicy.php (3)
app/Models/Permission.php (1)
Permission(11-221)app/Policies/Server/ActivityLogPolicy.php (1)
viewAny(12-15)app/Policies/Server/AllocationPolicy.php (4)
viewAny(13-16)create(18-21)edit(23-26)delete(28-31)
app/Policies/Server/FilePolicy.php (2)
app/Models/Permission.php (1)
Permission(11-221)app/Policies/Server/AllocationPolicy.php (4)
viewAny(13-16)create(18-21)edit(23-26)delete(28-31)
app/Policies/Server/AllocationPolicy.php (7)
app/Models/Permission.php (1)
Permission(11-221)app/Policies/Server/ActivityLogPolicy.php (1)
viewAny(12-15)app/Policies/Server/BackupPolicy.php (3)
viewAny(13-16)create(18-21)delete(23-26)app/Policies/Server/DatabasePolicy.php (4)
viewAny(13-16)create(23-26)edit(28-31)delete(33-36)app/Policies/Server/FilePolicy.php (4)
viewAny(13-16)create(18-21)edit(23-26)delete(28-31)app/Policies/Server/SchedulePolicy.php (4)
viewAny(13-16)create(18-21)edit(23-26)delete(28-31)app/Policies/Server/UserPolicy.php (4)
viewAny(13-16)create(18-21)edit(23-26)delete(28-31)
app/Providers/AppServiceProvider.php (1)
app/Models/User.php (2)
User(94-492)isRootAdmin(379-382)
🪛 PHPMD (2.15.0)
app/Policies/Server/DatabasePolicy.php
18-18: Avoid unused parameters such as '$record'. (undefined)
(UnusedFormalParameter)
28-28: Avoid unused parameters such as '$record'. (undefined)
(UnusedFormalParameter)
33-33: Avoid unused parameters such as '$record'. (undefined)
(UnusedFormalParameter)
app/Policies/Server/SchedulePolicy.php
23-23: Avoid unused parameters such as '$record'. (undefined)
(UnusedFormalParameter)
28-28: Avoid unused parameters such as '$record'. (undefined)
(UnusedFormalParameter)
app/Policies/Server/BackupPolicy.php
23-23: Avoid unused parameters such as '$record'. (undefined)
(UnusedFormalParameter)
app/Policies/Server/UserPolicy.php
23-23: Avoid unused parameters such as '$record'. (undefined)
(UnusedFormalParameter)
28-28: Avoid unused parameters such as '$record'. (undefined)
(UnusedFormalParameter)
app/Policies/Server/FilePolicy.php
23-23: Avoid unused parameters such as '$record'. (undefined)
(UnusedFormalParameter)
28-28: Avoid unused parameters such as '$record'. (undefined)
(UnusedFormalParameter)
app/Policies/Server/AllocationPolicy.php
23-23: Avoid unused parameters such as '$record'. (undefined)
(UnusedFormalParameter)
28-28: Avoid unused parameters such as '$record'. (undefined)
(UnusedFormalParameter)
app/Providers/AppServiceProvider.php
173-173: Avoid unused parameters such as '$ability'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (14)
app/Policies/Server/ServerPolicies.php (3)
3-3: LGTM! Good namespace organization.Moving server policies to the
App\Policies\Servernamespace improves code organization and aligns with the broader restructuring effort.
18-45: Authorization logic is well-structured.The
before()method correctly implements the authorization hierarchy:
- Owners receive full permissions
- Subusers are authorized based on their permission array
- Node targeting is enforced as a hard requirement
- Defers to default policy methods when none of the special cases apply
Note: When a subuser exists but lacks the specific permission, the method falls through to the node check and then returns
null. This delegates the final decision to individual policy methods (or the DefaultPolicies trait), which appears intentional for the authorization flow.
11-11: The DefaultPolicies trait correctly implements deny-by-default behavior for undefined policy methods.The trait's
__call()method intercepts any undefined method calls and returnsvoid(implicitly falsy), which causes Laravel's authorization to deny access. This is the intended "hack" to prevent unintended authorization bypasses. The implementation is secure and no changes are needed.app/Policies/Admin/RolePolicy.php (1)
3-3: Namespace move looks good.No behavioral change; aligns with Admin-scoped policies.
app/Policies/Admin/ApiKeyPolicy.php (1)
3-3: LGTM on namespace shift.Consistent with Admin policy layout.
app/Policies/Admin/EggPolicy.php (1)
3-3: OK to move under Admin namespace.Matches the new policy resolution scheme.
app/Policies/Admin/WebhookConfigurationPolicy.php (1)
3-3: Namespace adjustment approved.Consistent with the Admin policy grouping.
app/Policies/Admin/NodePolicy.php (1)
3-3: LGTM! Clean namespace refactor.The namespace change organizes admin policies into a dedicated subdirectory, aligning with the broader policy reorganization in this PR.
app/Policies/Admin/DatabaseHostPolicy.php (1)
3-3: LGTM! Consistent namespace refactor.The namespace change aligns with the policy reorganization pattern applied across all admin policies.
app/Policies/Admin/UserPolicy.php (1)
3-3: LGTM! Namespace refactor.Consistent with the policy reorganization moving admin policies to
App\Policies\Admin.app/Providers/AppServiceProvider.php (2)
173-173: LGTM! Cleaner arrow function syntax.The refactor to an arrow function is more concise while preserving the same logic (root admins bypass all checks).
Note: The static analysis warning about unused
$abilityis a false positive—Laravel'sGate::beforesignature requires this parameter.
175-188: LGTM! Dynamic policy resolution aligns with namespace refactors.The dynamic policy resolution based on the active Filament panel enables per-panel authorization logic. The implementation properly checks for class existence and skips when the panel ID is 'App', which complements the namespace reorganization of policies into
AdminandServersubdirectories.app/Policies/Server/BackupPolicy.php (1)
13-26: LGTM! Consistent server policy implementation.The policy methods correctly check permissions against the Filament tenant context, following the same pattern as other server policies in this PR.
Note: The static analysis warning about unused
$recordis likely a false positive—the parameter may be required for Laravel's policy method resolution or reserved for future use.app/Policies/Server/FilePolicy.php (1)
13-31: LGTM! Consistent file policy implementation.The policy methods correctly delegate to permission checks with appropriate
ACTION_FILE_*constants, following the same pattern asAllocationPolicyand other server policies.Note: The static analysis warnings about unused
$recordparameters are likely false positives—these parameters may be required by Laravel's policy method resolution or reserved for future enhancements.
rmartinoscar
force-pushed
the
chore/policies
branch
from
9127b9e to
42ce06e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (7)
app/Policies/Server/UserPolicy.php (2)
11-11: Consider removing unused$modelNameproperty.The
$modelNameproperty is not referenced in this policy class. Unless used by reflection (which doesn't appear to be the case), it should be removed.Apply this diff if unused:
- protected string $modelName = 'user'; - public function viewAny(): bool
13-31: Add null coalescing to prevent type errors when user is unauthenticated.The nullsafe operator
user()?->can(...)returnsnullwhenuser()isnull, violating theboolreturn type and potentially causing a fatal type error.Apply this diff:
public function viewAny(): bool { - return user()?->can(Permission::ACTION_USER_READ, Filament::getTenant()); + return user()?->can(Permission::ACTION_USER_READ, Filament::getTenant()) ?? false; } public function create(): bool { - return user()?->can(Permission::ACTION_USER_CREATE, Filament::getTenant()); + return user()?->can(Permission::ACTION_USER_CREATE, Filament::getTenant()) ?? false; } public function edit(Model $record): bool { - return user()?->can(Permission::ACTION_USER_UPDATE, Filament::getTenant()); + return user()?->can(Permission::ACTION_USER_UPDATE, Filament::getTenant()) ?? false; } public function delete(Model $record): bool { - return user()?->can(Permission::ACTION_USER_DELETE, Filament::getTenant()); + return user()?->can(Permission::ACTION_USER_DELETE, Filament::getTenant()) ?? false; }app/Filament/Server/Resources/Schedules/ScheduleResource.php (1)
338-339: Fix inverted authorization logic for ViewAction.The
ViewActionis currently hidden when edit is allowed, which is backwards. It should be hidden when edit is not allowed.Apply this diff:
ViewAction::make() - ->hidden(fn ($record) => static::getEditAuthorizationResponse($record)->allowed()), + ->hidden(fn ($record) => !static::getEditAuthorizationResponse($record)->allowed()),app/Policies/Server/SchedulePolicy.php (1)
13-31: Coalesce null to false to satisfy bool return type.All four methods use
user()?->can(...)which returnsnullwhen the user is not authenticated, violating theboolreturn type declaration. This will cause a type error in strict mode.Apply this diff to coalesce to
false:public function viewAny(): bool { - return user()?->can(Permission::ACTION_SCHEDULE_READ, Filament::getTenant()); + return user()?->can(Permission::ACTION_SCHEDULE_READ, Filament::getTenant()) ?? false; } public function create(): bool { - return user()?->can(Permission::ACTION_SCHEDULE_CREATE, Filament::getTenant()); + return user()?->can(Permission::ACTION_SCHEDULE_CREATE, Filament::getTenant()) ?? false; } public function edit(Model $record): bool { - return user()?->can(Permission::ACTION_SCHEDULE_UPDATE, Filament::getTenant()); + return user()?->can(Permission::ACTION_SCHEDULE_UPDATE, Filament::getTenant()) ?? false; } public function delete(Model $record): bool { - return user()?->can(Permission::ACTION_SCHEDULE_DELETE, Filament::getTenant()); + return user()?->can(Permission::ACTION_SCHEDULE_DELETE, Filament::getTenant()) ?? false; }app/Policies/Server/AllocationPolicy.php (1)
13-31: Coalesce null to false to satisfy bool return type.All four methods use
user()?->can(...)which returnsnullwhen the user is not authenticated, violating theboolreturn type declaration.Apply this diff:
public function viewAny(): bool { - return user()?->can(Permission::ACTION_ALLOCATION_READ, Filament::getTenant()); + return user()?->can(Permission::ACTION_ALLOCATION_READ, Filament::getTenant()) ?? false; } public function create(): bool { - return user()?->can(Permission::ACTION_ALLOCATION_CREATE, Filament::getTenant()); + return user()?->can(Permission::ACTION_ALLOCATION_CREATE, Filament::getTenant()) ?? false; } public function edit(Model $record): bool { - return user()?->can(Permission::ACTION_ALLOCATION_UPDATE, Filament::getTenant()); + return user()?->can(Permission::ACTION_ALLOCATION_UPDATE, Filament::getTenant()) ?? false; } public function delete(Model $record): bool { - return user()?->can(Permission::ACTION_ALLOCATION_DELETE, Filament::getTenant()); + return user()?->can(Permission::ACTION_ALLOCATION_DELETE, Filament::getTenant()) ?? false; }app/Policies/Server/ActivityLogPolicy.php (1)
13-21: Coalesce null to false to satisfy bool return type.Both methods use
user()?->can(...)which returnsnullwhen the user is not authenticated, violating theboolreturn type declaration.Apply this diff:
public function viewAny(): bool { - return user()?->can(Permission::ACTION_ACTIVITY_READ, Filament::getTenant()); + return user()?->can(Permission::ACTION_ACTIVITY_READ, Filament::getTenant()) ?? false; } public function view(Model $model): bool { - return user()?->can(Permission::ACTION_ACTIVITY_READ, Filament::getTenant()); + return user()?->can(Permission::ACTION_ACTIVITY_READ, Filament::getTenant()) ?? false; }app/Policies/Server/DatabasePolicy.php (1)
13-36: Coalesce null to false to satisfy bool return type.All five methods use
user()?->can(...)which returnsnullwhen the user is not authenticated, violating theboolreturn type declaration.Apply this diff:
public function viewAny(): bool { - return user()?->can(Permission::ACTION_DATABASE_READ, Filament::getTenant()); + return user()?->can(Permission::ACTION_DATABASE_READ, Filament::getTenant()) ?? false; } public function view(Model $record): bool { - return user()?->can(Permission::ACTION_DATABASE_READ, Filament::getTenant()); + return user()?->can(Permission::ACTION_DATABASE_READ, Filament::getTenant()) ?? false; } public function create(): bool { - return user()?->can(Permission::ACTION_DATABASE_CREATE, Filament::getTenant()); + return user()?->can(Permission::ACTION_DATABASE_CREATE, Filament::getTenant()) ?? false; } public function edit(Model $record): bool { - return user()?->can(Permission::ACTION_DATABASE_UPDATE, Filament::getTenant()); + return user()?->can(Permission::ACTION_DATABASE_UPDATE, Filament::getTenant()) ?? false; } public function delete(Model $record): bool { - return user()?->can(Permission::ACTION_DATABASE_DELETE, Filament::getTenant()); + return user()?->can(Permission::ACTION_DATABASE_DELETE, Filament::getTenant()) ?? false; }
🧹 Nitpick comments (6)
app/Policies/Server/BackupPolicy.php (1)
11-11: Consider removing unused$modelNameproperty.The
$modelNameproperty is not referenced anywhere in this policy class. Unless it's used by reflection or a parent trait (which doesn't appear to be the case), it should be removed.Apply this diff if unused:
- protected string $modelName = 'backup'; - public function viewAny(): boolapp/Policies/Server/FilePolicy.php (1)
11-11: Consider removing unused$modelNameproperty.The
$modelNameproperty is not referenced in this policy class. Unless used by reflection (which doesn't appear to be the case), it should be removed.Apply this diff if unused:
- protected string $modelName = 'file'; - public function viewAny(): boolapp/Policies/Server/SchedulePolicy.php (1)
23-26: Unused$recordparameter is acceptable for Laravel policy contract.The static analysis tool flags the
$recordparameter as unused in bothedit()anddelete()methods. This is acceptable because:
- Laravel policy methods follow standard signatures that include the model parameter
- The permission checks are tenant-scoped via
Filament::getTenant()rather than record-scoped- Maintaining the parameter preserves consistency with Laravel's policy contract
Also applies to: 28-31
app/Policies/Server/AllocationPolicy.php (1)
23-26: Unused$recordparameter is acceptable for Laravel policy contract.The
$recordparameter inedit()anddelete()follows Laravel's standard policy signature. Since permission checks are tenant-scoped viaFilament::getTenant(), the parameter remains unused but maintains API consistency.Also applies to: 28-31
app/Policies/Server/ActivityLogPolicy.php (1)
18-21: Unused$modelparameter is acceptable for Laravel policy contract.The
$modelparameter inview()follows Laravel's standard policy signature. Since the permission check is tenant-scoped, the parameter remains unused but maintains API consistency.app/Policies/Server/DatabasePolicy.php (1)
18-21: Unused$recordparameter is acceptable for Laravel policy contract.The
$recordparameter inview(),edit(), anddelete()methods follows Laravel's standard policy signature. Since permission checks are tenant-scoped viaFilament::getTenant(), the parameters remain unused but maintain API consistency.Also applies to: 28-31, 33-36
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
app/Filament/Admin/Resources/DatabaseHosts/DatabaseHostResource.php(1 hunks)app/Filament/Admin/Resources/Mounts/MountResource.php(1 hunks)app/Filament/Admin/Resources/Roles/RoleResource.php(1 hunks)app/Filament/Admin/Resources/Users/UserResource.php(1 hunks)app/Filament/Admin/Resources/Webhooks/WebhookResource.php(1 hunks)app/Filament/Server/Resources/Activities/ActivityResource.php(0 hunks)app/Filament/Server/Resources/Allocations/AllocationResource.php(0 hunks)app/Filament/Server/Resources/Backups/BackupResource.php(0 hunks)app/Filament/Server/Resources/Databases/DatabaseResource.php(0 hunks)app/Filament/Server/Resources/Files/FileResource.php(0 hunks)app/Filament/Server/Resources/Schedules/ScheduleResource.php(1 hunks)app/Filament/Server/Resources/Users/UserResource.php(0 hunks)app/Policies/Admin/ApiKeyPolicy.php(1 hunks)app/Policies/Admin/DatabaseHostPolicy.php(1 hunks)app/Policies/Admin/DefaultPolicies.php(1 hunks)app/Policies/Admin/EggPolicy.php(1 hunks)app/Policies/Admin/MountPolicy.php(1 hunks)app/Policies/Admin/NodePolicy.php(1 hunks)app/Policies/Admin/RolePolicy.php(1 hunks)app/Policies/Admin/UserPolicy.php(1 hunks)app/Policies/Admin/WebhookConfigurationPolicy.php(1 hunks)app/Policies/DatabasePolicy.php(0 hunks)app/Policies/Server/ActivityLogPolicy.php(1 hunks)app/Policies/Server/AllocationPolicy.php(1 hunks)app/Policies/Server/BackupPolicy.php(1 hunks)app/Policies/Server/DatabasePolicy.php(1 hunks)app/Policies/Server/DefaultPolicies.php(1 hunks)app/Policies/Server/FilePolicy.php(1 hunks)app/Policies/Server/SchedulePolicy.php(1 hunks)app/Policies/Server/ServerPolicies.php(1 hunks)app/Policies/Server/UserPolicy.php(1 hunks)app/Providers/AppServiceProvider.php(2 hunks)
💤 Files with no reviewable changes (7)
- app/Filament/Server/Resources/Users/UserResource.php
- app/Filament/Server/Resources/Activities/ActivityResource.php
- app/Filament/Server/Resources/Backups/BackupResource.php
- app/Filament/Server/Resources/Databases/DatabaseResource.php
- app/Filament/Server/Resources/Files/FileResource.php
- app/Filament/Server/Resources/Allocations/AllocationResource.php
- app/Policies/DatabasePolicy.php
🚧 Files skipped from review as they are similar to previous changes (5)
- app/Policies/Admin/DatabaseHostPolicy.php
- app/Policies/Admin/EggPolicy.php
- app/Policies/Admin/ApiKeyPolicy.php
- app/Policies/Admin/WebhookConfigurationPolicy.php
- app/Policies/Admin/DefaultPolicies.php
🧰 Additional context used
🧬 Code graph analysis (8)
app/Policies/Server/AllocationPolicy.php (2)
app/Models/Permission.php (1)
Permission(11-221)app/Policies/Server/DatabasePolicy.php (4)
viewAny(13-16)create(23-26)edit(28-31)delete(33-36)
app/Providers/AppServiceProvider.php (5)
app/Policies/Admin/DatabaseHostPolicy.php (1)
before(14-28)app/Policies/Admin/MountPolicy.php (1)
before(14-28)app/Policies/Admin/NodePolicy.php (1)
before(14-26)app/Policies/Server/ServerPolicies.php (1)
before(18-45)app/Models/User.php (2)
User(94-492)isRootAdmin(379-382)
app/Policies/Server/ActivityLogPolicy.php (3)
app/Models/Permission.php (1)
Permission(11-221)app/Policies/Server/AllocationPolicy.php (1)
viewAny(13-16)app/Policies/Server/DatabasePolicy.php (2)
viewAny(13-16)view(18-21)
app/Policies/Server/UserPolicy.php (2)
app/Models/Permission.php (1)
Permission(11-221)app/Policies/Server/AllocationPolicy.php (4)
viewAny(13-16)create(18-21)edit(23-26)delete(28-31)
app/Policies/Server/FilePolicy.php (3)
app/Models/Permission.php (1)
Permission(11-221)app/Policies/Server/ActivityLogPolicy.php (1)
viewAny(13-16)app/Policies/Server/AllocationPolicy.php (4)
viewAny(13-16)create(18-21)edit(23-26)delete(28-31)
app/Policies/Server/BackupPolicy.php (5)
app/Models/Permission.php (1)
Permission(11-221)app/Policies/Server/ActivityLogPolicy.php (1)
viewAny(13-16)app/Policies/Server/AllocationPolicy.php (3)
viewAny(13-16)create(18-21)delete(28-31)app/Policies/Server/DatabasePolicy.php (3)
viewAny(13-16)create(23-26)delete(33-36)app/Policies/Server/FilePolicy.php (3)
viewAny(13-16)create(18-21)delete(28-31)
app/Policies/Server/DatabasePolicy.php (4)
app/Models/Permission.php (1)
Permission(11-221)app/Policies/Server/ActivityLogPolicy.php (2)
viewAny(13-16)view(18-21)app/Policies/Server/AllocationPolicy.php (4)
viewAny(13-16)create(18-21)edit(23-26)delete(28-31)app/Policies/Server/FilePolicy.php (4)
viewAny(13-16)create(18-21)edit(23-26)delete(28-31)
app/Policies/Server/SchedulePolicy.php (2)
app/Models/Permission.php (1)
Permission(11-221)app/Policies/Server/AllocationPolicy.php (4)
viewAny(13-16)create(18-21)edit(23-26)delete(28-31)
🪛 PHPMD (2.15.0)
app/Policies/Server/AllocationPolicy.php
23-23: Avoid unused parameters such as '$record'. (undefined)
(UnusedFormalParameter)
28-28: Avoid unused parameters such as '$record'. (undefined)
(UnusedFormalParameter)
app/Providers/AppServiceProvider.php
173-173: Avoid unused parameters such as '$ability'. (undefined)
(UnusedFormalParameter)
app/Policies/Server/ActivityLogPolicy.php
18-18: Avoid unused parameters such as '$model'. (undefined)
(UnusedFormalParameter)
app/Policies/Server/UserPolicy.php
23-23: Avoid unused parameters such as '$record'. (undefined)
(UnusedFormalParameter)
28-28: Avoid unused parameters such as '$record'. (undefined)
(UnusedFormalParameter)
app/Policies/Server/FilePolicy.php
23-23: Avoid unused parameters such as '$record'. (undefined)
(UnusedFormalParameter)
28-28: Avoid unused parameters such as '$record'. (undefined)
(UnusedFormalParameter)
app/Policies/Server/BackupPolicy.php
23-23: Avoid unused parameters such as '$record'. (undefined)
(UnusedFormalParameter)
app/Policies/Server/DatabasePolicy.php
18-18: Avoid unused parameters such as '$record'. (undefined)
(UnusedFormalParameter)
28-28: Avoid unused parameters such as '$record'. (undefined)
(UnusedFormalParameter)
33-33: Avoid unused parameters such as '$record'. (undefined)
(UnusedFormalParameter)
app/Policies/Server/SchedulePolicy.php
23-23: Avoid unused parameters such as '$record'. (undefined)
(UnusedFormalParameter)
28-28: Avoid unused parameters such as '$record'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (14)
app/Filament/Admin/Resources/DatabaseHosts/DatabaseHostResource.php (1)
95-95: LGTM! Authorization response migration implemented correctly.The ViewAction visibility now uses
getEditAuthorizationResponse($record)->allowed()instead of the deprecatedcanEdit($record)method, aligning with the PR's objective to adopt policy response objects for authorization.app/Filament/Admin/Resources/Users/UserResource.php (1)
133-133: LGTM! Consistent with authorization response pattern.ViewAction visibility correctly migrated to use authorization response method.
app/Policies/Admin/RolePolicy.php (1)
3-3: LGTM! Policy namespace reorganization.The namespace change from
App\PoliciestoApp\Policies\Adminis part of a broader policy reorganization to separate admin and server policies. No behavioral changes.app/Filament/Admin/Resources/Webhooks/WebhookResource.php (1)
101-101: LGTM! Authorization response pattern applied correctly.app/Policies/Admin/NodePolicy.php (1)
3-3: LGTM! Namespace refactor consistent with policy reorganization.app/Policies/Admin/MountPolicy.php (1)
3-3: LGTM! Policy namespace reorganization applied consistently.app/Filament/Admin/Resources/Roles/RoleResource.php (1)
101-101: LGTM! Authorization response migration implemented correctly.app/Filament/Admin/Resources/Mounts/MountResource.php (1)
98-98: LGTM! Authorization response pattern applied consistently.Excellent work on this PR! All resource files consistently migrate from
canEdit()togetEditAuthorizationResponse()->allowed(), and all policy namespaces are properly reorganized. The changes align perfectly with the PR objectives.app/Policies/Server/ServerPolicies.php (2)
3-3: LGTM! Namespace refactored to align with Admin/Server policy structure.The move to
App\Policies\Serveris consistent with the new policy organization approach.
11-11: LGTM! DefaultPolicies trait ensures before() is always invoked.The trait provides a
__callfallback so Laravel's authorization system will callbefore()even when no matching policy method exists.app/Policies/Server/DefaultPolicies.php (1)
5-18: LGTM! Trait correctly implements __call workaround.The
__callmethod signature is correct witharray $arguments, and the no-op implementation appropriately ensures Laravel invokesbefore()for all authorization checks, even when specific policy methods don't exist.app/Policies/Admin/UserPolicy.php (1)
3-3: LGTM! Namespace refactored to Admin policy structure.The move to
App\Policies\Adminaligns with the new policy organization separating Admin and Server contexts.app/Providers/AppServiceProvider.php (2)
173-173: LGTM! Simplified Gate::before to arrow function.The arrow function is more concise and correctly returns
truefor root admins, allowing other users to proceed through normal authorization.Note: The
$abilityparameter is required by theGate::beforesignature even though it's unused here.
175-188: LGTM! Dynamic policy resolution enables panel-specific policies.The
Gate::guessPolicyNamesUsingcallback correctly maps models to panel-specific policy classes (App\Policies\Admin\*orApp\Policies\Server\*), supporting the new authorization structure.
| public function viewAny(): bool | ||
| { | ||
| return user()?->can(Permission::ACTION_ACTIVITY_READ, Filament::getTenant()); | ||
| } |
There was a problem hiding this comment.
The user should be passed as param, like the other (admin) policies.
This is not always the logged-in user.
panel/app/Policies/DefaultPolicies.php
Lines 10 to 13 in d1a808a
|
|
||
| class ActivityLogPolicy | ||
| { | ||
| protected string $modelName = 'activityLog'; |
There was a problem hiding this comment.
I don't think the modelName is needed?
| public function __call(string $name, array $arguments): void | ||
| { | ||
| // do nothing | ||
| } |
There was a problem hiding this comment.
Why was this moved to it's own class/ trait?
|
|
||
| if (class_exists($class)) { | ||
| return $class; | ||
| } |
There was a problem hiding this comment.
Isn't this missing a return null; or something at the end?
Closes #1831
Made this a Draft cause Filament doesn't seem to call them everywhere atm.