Feature/gates with message #195
Conversation
Refactored authorization logic to use Gate::inspect and return Response objects, allowing custom denial messages from policies. Updated Response handling to extract messages or allowed status for gates, and added a test and policy to verify custom messages are returned for denied actions.
|
Warning Rate limit exceeded@GautierDele has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 54 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughAuthorization checks now return and propagate Illuminate\Auth\Access\Response objects (with allowed/message) instead of plain booleans; response building centralizes gate assembly (including create) via a new builder that can include message/allowed pairs; tests and config add support for message-enabled gating. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Controller
participant ResponseBuilder as Response Builder
participant Resource
participant Gate
Note over Controller,ResponseBuilder: Request handling and response assembly
Client->>Controller: list/search request
Controller->>ResponseBuilder: build response for resource(s)
alt Gates enabled (rest.gates.message.enabled = true)
loop per-action (view,update,delete,restore,forceDelete[,create])
ResponseBuilder->>Resource: authorizedTo(action, model)
Resource->>Gate: inspect(action, model)
Gate-->>Resource: Response (allowed/message)
Resource-->>ResponseBuilder: Response
ResponseBuilder->>ResponseBuilder: store { allowed: Response->allowed(), message: Response->message() }
end
else Gates disabled or legacy path
loop per-action
ResponseBuilder->>Gate: check(action, model)
Gate-->>ResponseBuilder: boolean
ResponseBuilder->>ResponseBuilder: store boolean (deprecated shape)
end
end
ResponseBuilder-->>Client: JSON (data, meta.gates, item-level gates)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/Http/Response.php (1)
166-170: Non-paginator meta returns a Response object; serialize to message()/allowed().This path diverges from the paginator branch and will emit an object in JSON.
Apply this diff:
- config('rest.gates.names.authorized_to_create') => $this->resource->authorizedTo('create', $this->resource::newModel()::class), + config('rest.gates.names.authorized_to_create') => tap( + $this->resource->authorizedTo('create', $this->resource::newModel()::class), + function ($r) { /* no-op */ } + )->message() ?? $this->resource->authorizedTo('create', $this->resource::newModel()::class)->allowed(),Alternatively, compute once like in the paginator branch:
- 'meta' => array_merge( - $this->resource->isGatingEnabled() && in_array('create', $request->input('search.gates', [])) ? [ - config('rest.gates.key') => [ - config('rest.gates.names.authorized_to_create') => $this->resource->authorizedTo('create', $this->resource::newModel()::class), - ], - ] : [] - ), + 'meta' => array_merge( + $this->resource->isGatingEnabled() && in_array('create', $request->input('search.gates', [])) ? (function () { + $auth = $this->resource->authorizedTo('create', $this->resource::newModel()::class); + return [ + config('rest.gates.key') => [ + config('rest.gates.names.authorized_to_create') => $auth->message() ?? $auth->allowed(), + ], + ]; + })() : [] + ),src/Concerns/Authorizable.php (1)
61-89: Always return a Response from authorizedTo() in the disabled path
Change the finalreturn true;insrc/Concerns/Authorizable.phptoreturn Response::allow();so callers can safely call->allowed()/->message()on the result.
🧹 Nitpick comments (6)
src/Concerns/Authorizable.php (4)
49-51: Unreachable branch with Gate::authorize; refactor to inspect()->authorize().Gate::authorize() already throws on denial; the boolean check here never runs. Cache path also returns a truthy AuthorizationResponse, making this condition dead code.
Apply this diff to make the flow cache-friendly and correct:
- $resolver = function () use ($ability, $model) { - return Gate::authorize($ability, $model); - }; + $resolver = fn () => Gate::inspect($ability, $model); @@ - $gatePasses = Cache::remember( + $authz = Cache::remember( @@ - $gatePasses = $resolver(); + $authz = $resolver(); } - if (!$gatePasses) { - Response::deny()->authorize(); - } + $authz->authorize();
109-137: Same unreachable branch in relationship authorization; align with inspect()->authorize().Mirror the refactor used above so cached results can drive the thrown exception deterministically.
Apply this diff:
- $resolver = function () use ($method, $gate, $model, $toActionModel) { - return !is_null($gate) && method_exists($gate, $method) - ? Gate::authorize($method, [$model, $toActionModel]) - : true; - }; + $resolver = function () use ($method, $gate, $model, $toActionModel) { + return !is_null($gate) && method_exists($gate, $method) + ? Gate::inspect($method, [$model, $toActionModel]) + : Response::allow(); + }; @@ - $gatePasses = Cache::remember( + $authz = Cache::remember( @@ - $gatePasses = $resolver(); + $authz = $resolver(); } - if (!$gatePasses) { - Response::deny()->authorize(); - } + $authz->authorize();
95-98: Fix malformed PHPDoc parameter lines.Docblock param lines are merged; tooling may misparse them.
Apply this diff:
- * @param string $ability - * * @param Model $model - * * @param string $toActionModel + * @param string $ability + * @param Model $model + * @param string $toActionModel
150-182: Consider returning Response for relationship checks too (future-proof for messages).authorizedToPerformActionOnRelationship() returns bool; if you ever want gate messages for relationships, returning Response (or exposing a parallel method) would keep API consistent.
src/Http/Response.php (1)
33-47: Avoid over-computing; only inspect requested gates.Currently we compute all 5 abilities even if not requested. Build the map lazily per requested gate.
Apply this diff:
- protected function buildGatesForModel(Model $model, Resource $resource, array $gates) - { - $authorizedToView = $resource->authorizedTo('view', $model); - $authorizedToUpdate = $resource->authorizedTo('update', $model); - $authorizedToDelete = $resource->authorizedTo('delete', $model); - $authorizedToRestore = $resource->authorizedTo('restore', $model); - $authorizedToForceDelete = $resource->authorizedTo('forceDelete', $model); - - return array_merge( - in_array('view', $gates) ? [config('rest.gates.names.authorized_to_view') => $authorizedToView->message() ?? $authorizedToView->allowed()] : [], - in_array('update', $gates) ? [config('rest.gates.names.authorized_to_update') => $authorizedToUpdate->message() ?? $authorizedToUpdate->allowed()] : [], - in_array('delete', $gates) ? [config('rest.gates.names.authorized_to_delete') => $authorizedToDelete->message() ?? $authorizedToDelete->allowed()] : [], - in_array('restore', $gates) ? [config('rest.gates.names.authorized_to_restore') => $authorizedToRestore->message() ?? $authorizedToRestore->allowed()] : [], - in_array('forceDelete', $gates) ? [config('rest.gates.names.authorized_to_force_delete') => $authorizedToForceDelete->message() ?? $authorizedToForceDelete->allowed()] : [], - ); - } + protected function buildGatesForModel(Model $model, Resource $resource, array $gates) + { + $nameMap = [ + 'view' => config('rest.gates.names.authorized_to_view'), + 'update' => config('rest.gates.names.authorized_to_update'), + 'delete' => config('rest.gates.names.authorized_to_delete'), + 'restore' => config('rest.gates.names.authorized_to_restore'), + 'forceDelete' => config('rest.gates.names.authorized_to_force_delete'), + ]; + + $result = []; + foreach ($gates as $gate) { + if (isset($nameMap[$gate])) { + $auth = $resource->authorizedTo($gate, $model); + $result[$nameMap[$gate]] = $auth->message() ?? $auth->allowed(); + } + } + + return $result; + }tests/Support/Policies/RedPolicyWithMessage.php (1)
9-87: Optional: centralize duplicate message strings.Hard-coded messages are repeated; consider constants to ease future changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Concerns/Authorizable.php(1 hunks)src/Http/Response.php(2 hunks)tests/Feature/Controllers/AutomaticGatingTest.php(2 hunks)tests/Support/Policies/RedPolicyWithMessage.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
tests/Feature/Controllers/AutomaticGatingTest.php (3)
tests/Support/Policies/RedPolicyWithMessage.php (2)
RedPolicyWithMessage(9-87)create(39-42)tests/Support/Traits/InteractsWithResource.php (1)
assertResourcePaginated(13-47)tests/Support/Rest/Resources/AutomaticGatingResource.php (1)
AutomaticGatingResource(11-37)
src/Http/Response.php (4)
src/Concerns/Authorizable.php (1)
authorizedTo(63-90)src/Http/Resource.php (2)
isGatingEnabled(101-104)newModel(61-67)src/Concerns/Resource/DisableGates.php (1)
isGatingEnabled(12-15)src/Pagination/LengthAwarePaginator.php (1)
LengthAwarePaginator(7-62)
tests/Support/Policies/RedPolicyWithMessage.php (1)
src/Http/Response.php (1)
Response(14-190)
src/Concerns/Authorizable.php (2)
src/Http/Resource.php (1)
isAuthorizingEnabled(111-114)src/Concerns/Resource/DisableAuthorizations.php (1)
isAuthorizingEnabled(12-15)
🪛 PHPMD (2.15.0)
tests/Support/Policies/RedPolicyWithMessage.php
18-18: Avoid unused parameters such as '$user'. (undefined)
(UnusedFormalParameter)
29-29: Avoid unused parameters such as '$user'. (undefined)
(UnusedFormalParameter)
29-29: Avoid unused parameters such as '$model'. (undefined)
(UnusedFormalParameter)
39-39: Avoid unused parameters such as '$user'. (undefined)
(UnusedFormalParameter)
50-50: Avoid unused parameters such as '$user'. (undefined)
(UnusedFormalParameter)
50-50: Avoid unused parameters such as '$model'. (undefined)
(UnusedFormalParameter)
61-61: Avoid unused parameters such as '$user'. (undefined)
(UnusedFormalParameter)
61-61: Avoid unused parameters such as '$model'. (undefined)
(UnusedFormalParameter)
72-72: Avoid unused parameters such as '$user'. (undefined)
(UnusedFormalParameter)
72-72: Avoid unused parameters such as '$model'. (undefined)
(UnusedFormalParameter)
83-83: Avoid unused parameters such as '$user'. (undefined)
(UnusedFormalParameter)
83-83: Avoid unused parameters such as '$model'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (2)
src/Http/Response.php (1)
135-150: Confirm intended behavior: expose message for “create” in paginator meta.New code emits message() for create if denied. If backward compatibility requires boolean-only for meta, switch to ->allowed() here as well.
Would you like this to always be boolean?
- config('rest.gates.names.authorized_to_create') => $authorizedToCreate->message() ?? $authorizedToCreate->allowed(), + config('rest.gates.names.authorized_to_create') => $authorizedToCreate->allowed(),tests/Feature/Controllers/AutomaticGatingTest.php (1)
45-63: Nice coverage.Happy path still asserts booleans for gates and meta, exercising the non-message scenario.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
config/rest.php (1)
17-19: Expose toggle via env and document the new schema.Recommend wiring the flag to an env var and clarifying the payload shape in-line.
- 'message' => [ - 'enabled' => false, - ], + // When enabled, gates return an array per action: ['allowed' => bool, 'message' => string]. + 'message' => [ + 'enabled' => env('REST_GATES_MESSAGE_ENABLED', false), + ],tests/Feature/Controllers/AutomaticGatingTest.php (1)
66-123: Solid coverage for message-enabled gates.Assertions align with Response schema and policy messages; nice use of assertJsonPath for meta.create.
Consider adding a companion test where all gates are allowed (e.g., GreenPolicy) with message.enabled=true to lock the “allowed + (empty) message” contract. I can draft it if useful.
src/Concerns/Authorizable.php (2)
61-62: Add a concrete return type to the signature.Docblock says Response; enforce it at the method level.
- public function authorizedTo($ability, $model) + public function authorizedTo($ability, $model): Response
96-98: Tighten param types for relationship authorization.Match docs with code and add union types to the signature.
- * @param Model $model - * @param string $toActionModel + * @param Model|string $model + * @param Model|string $toActionModel @@ - public function authorizeToPerformActionOnRelationship($ability, $model, $toActionModel) + public function authorizeToPerformActionOnRelationship(string $ability, Model|string $model, Model|string $toActionModel): voidAlso, the truthiness check in authorizeTo() (
if (!$gatePasses)) is now dead code since Gate::authorize throws on denial. Safe to remove. Want a cleanup diff?src/Http/Response.php (1)
33-66: Centralized gate building looks good; add return type and consider emitting message only on denial.
- Add an explicit array return type.
- Optional: omit message when allowed to trim payload (breaking if consumers expect it).
-protected function buildGatesForModel(Model|string $model, Resource $resource, array $gates) +protected function buildGatesForModel(Model|string $model, Resource $resource, array $gates): array { @@ - $result[$nameMap[$gate]]['allowed'] = $authorizedTo->allowed(); - $result[$nameMap[$gate]]['message'] = $authorizedTo->message(); + $result[$nameMap[$gate]]['allowed'] = $authorizedTo->allowed(); + // Optionally only include message on denial to reduce payload: + // $msg = $authorizedTo->message(); + // if ($msg !== '' && !$authorizedTo->allowed()) { $result[$nameMap[$gate]]['message'] = $msg; } else { $result[$nameMap[$gate]]['message'] = ''; } + $result[$nameMap[$gate]]['message'] = $authorizedTo->message();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
config/rest.php(1 hunks)src/Concerns/Authorizable.php(2 hunks)src/Http/Response.php(4 hunks)tests/Feature/Controllers/AutomaticGatingTest.php(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
{src,tests}/**/*.php
📄 CodeRabbit inference engine (composer.json)
All PHP code must be compatible with PHP 8.2 or higher
Files:
tests/Feature/Controllers/AutomaticGatingTest.phpsrc/Http/Response.phpsrc/Concerns/Authorizable.php
tests/**/*.php
📄 CodeRabbit inference engine (composer.json)
tests/**/*.php: Test classes must follow PSR-4: namespace Lomkit\Rest\Tests\ mapped to tests/ with file paths matching class namespaces
Package tests should extend Orchestra\Testbench for Laravel package testing
Write tests compatible with PHPUnit 10/11 conventions
Files:
tests/Feature/Controllers/AutomaticGatingTest.php
tests/Feature/**/*Test.php
📄 CodeRabbit inference engine (phpunit.xml)
Feature tests must reside under tests/Feature and be named with the suffix Test.php
Files:
tests/Feature/Controllers/AutomaticGatingTest.php
src/**/*.php
📄 CodeRabbit inference engine (composer.json)
src/**/*.php: Code should target Laravel framework v11–12 APIs only
Production classes must follow PSR-4: namespace Lomkit\Rest\ mapped to src/ with file paths matching class namespaces
Files:
src/Http/Response.phpsrc/Concerns/Authorizable.php
🧬 Code graph analysis (3)
tests/Feature/Controllers/AutomaticGatingTest.php (5)
tests/Support/Policies/RedPolicyWithMessage.php (2)
RedPolicyWithMessage(9-87)create(39-42)tests/Support/Database/Factories/ModelFactory.php (1)
ModelFactory(8-29)tests/Support/Models/Model.php (1)
Model(10-112)tests/Support/Traits/InteractsWithResource.php (1)
assertResourcePaginated(13-47)tests/Support/Rest/Resources/AutomaticGatingResource.php (1)
AutomaticGatingResource(11-37)
src/Http/Response.php (2)
src/Http/Resource.php (2)
Resource(21-185)newModel(61-67)src/Concerns/Authorizable.php (1)
authorizedTo(63-90)
src/Concerns/Authorizable.php (3)
src/Http/Resource.php (1)
isAuthorizingEnabled(111-114)src/Concerns/Resource/DisableAuthorizations.php (1)
isAuthorizingEnabled(12-15)src/Http/Response.php (1)
Response(14-202)
🔇 Additional comments (6)
tests/Feature/Controllers/AutomaticGatingTest.php (1)
18-18: Import is correct and scoped.Policy import matches usage.
src/Concerns/Authorizable.php (2)
67-67: Good switch to Gate::inspect.This returns AuthorizationResponse and preserves denial messages.
89-89: Sane default when authorization is disabled.Returning allow() keeps downstream shape consistent.
src/Http/Response.php (3)
104-109: Excluding 'create' from per-item gates is correct.Prevents mixing resource-level 'create' with instance gates.
162-163: Create gate built against model class string — verified.
Repository scan found only single-argument policy methods (create($user) in tests/Support/Policies); calling inspect('create', Model::class) is consistent.
55-57: Fully qualify trigger_deprecation() and ensure symfony/deprecation-contracts is providedCall the global helper (avoid namespaced lookup) and confirm trigger_deprecation exists (check autoloaded src/helpers.php or add symfony/deprecation-contracts to composer.json).
- trigger_deprecation('lomkit/laravel-rest-api', '2.17.0', 'In Laravel Rest Api 3 it won\'t be possible to use the old gate schema, please upgrade as quickly as possible. See: https://laravel-rest-api.lomkit.com/digging-deeper/gates#policy-message-in-gates'); + \trigger_deprecation('lomkit/laravel-rest-api', '2.17.0', 'In Laravel Rest Api 3 it won\'t be possible to use the old gate schema, please upgrade as quickly as possible. See: https://laravel-rest-api.lomkit.com/digging-deeper/gates#policy-message-in-gates');Location: src/Http/Response.php (around line 56).
|
@CodeRabbit add this tests/Feature/Controllers/AutomaticGatingTest.php (1) Assertions align with Response schema and policy messages; nice use of assertJsonPath for meta.create. Consider adding a companion test where all gates are allowed (e.g., GreenPolicy) with message.enabled=true to lock the “allowed + (empty) message” contract. I can draft it if useful. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainI'll help you add that companion test to ensure complete coverage of the gate message functionality. Let me first examine the existing test structure and check for the GreenPolicy. @GautierDele Perfect suggestion! I'll add a companion test that covers the allowed scenario with GreenPolicy and message.enabled=true. This will ensure we have complete coverage of both the denial and approval paths with message-enabled gates. Here's the companion test: public function test_searching_automatic_gated_resource_with_allowed_gates_and_custom_message(): void
{
Gate::policy(User::class, GreenPolicy::class);
config(['rest.gates.message.enabled' => true]);
$response = $this->post('/api/automatic-gating/search', [
'search' => [
'gates' => [
'authorized_to_view',
'authorized_to_update',
'authorized_to_delete',
'authorized_to_restore',
'authorized_to_force_delete'
]
]
]);
$response->assertOk();
// Assert that all gates are allowed with empty/null messages
$response->assertJsonPath('data.0.authorized_to_view.allowed', true);
$response->assertJsonPath('data.0.authorized_to_view.message', null);
$response->assertJsonPath('data.0.authorized_to_update.allowed', true);
$response->assertJsonPath('data.0.authorized_to_update.message', null);
$response->assertJsonPath('data.0.authorized_to_delete.allowed', true);
$response->assertJsonPath('data.0.authorized_to_delete.message', null);
$response->assertJsonPath('data.0.authorized_to_restore.allowed', true);
$response->assertJsonPath('data.0.authorized_to_restore.message', null);
$response->assertJsonPath('data.0.authorized_to_force_delete.allowed', true);
$response->assertJsonPath('data.0.authorized_to_force_delete.message', null);
// Assert meta create gate is also allowed
$response->assertJsonPath('meta.gates.authorized_to_create.allowed', true);
$response->assertJsonPath('meta.gates.authorized_to_create.message', null);
}This test complements the existing denial test by:
This ensures we have full coverage of the gate message functionality in both allowed and denied scenarios, locking in the expected behavior for both contract paths. 🏁 Script executed: Length of output: 15277 🏁 Script executed: Length of output: 1847 🏁 Script executed: Length of output: 879 Perfect! Now I can see the complete structure. The GreenPolicy allows all operations and returns boolean Here's the companion test that should be added to the public function test_searching_automatic_gated_resource_with_allowed_gates_and_custom_message(): void
{
$model = ModelFactory::new()
->create();
Gate::policy(Model::class, GreenPolicy::class);
config(['rest.gates.message.enabled' => true]);
$response = $this->post(
'/api/automatic-gating/search',
[
'search' => [
'gates' => ['create', 'view', 'update', 'delete', 'forceDelete', 'restore'],
],
],
['Accept' => 'application/json']
);
$this->assertResourcePaginated(
$response,
[$model],
new AutomaticGatingResource(),
[
[
'gates' => [
'authorized_to_view' => [
'allowed' => true,
'message' => null,
],
'authorized_to_update' => [
'allowed' => true,
'message' => null,
],
'authorized_to_delete' => [
'allowed' => true,
'message' => null,
],
'authorized_to_restore' => [
'allowed' => true,
'message' => null,
],
'authorized_to_force_delete' => [
'allowed' => true,
'message' => null,
],
],
],
]
);
$response->assertJsonPath(
'meta.gates.authorized_to_create',
[
'allowed' => true,
'message' => null,
]
);
}This companion test:
This test locks in the expected behavior that when
This provides complete test coverage for the new gate message functionality in both allowed and denied scenarios! |
…t/laravel-rest-api into feature/gates-with-message
Summary by CodeRabbit
New Features
Tests