Feature/reworked package #5
Conversation
WalkthroughThe changes remove several GitHub issue templates and update CI configurations. Dependency versions in Composer have been raised, and access control configuration settings have been modified. The service provider and legacy traits for policies and queries were removed, while the core Control class was refactored to use static methods for control name resolution and permission checks. In addition, the Perimeter system was reworked by removing fixed properties and collection classes and introducing callback-based logic and a new OverlayPerimeter. A number of tests have been added, removed, or refactored to align with the new access control flow. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant P as ControlledPolicy
participant C as Control
participant M as Model
U->>P: Request action (e.g., "view")
P->>C: getControl() with model name
C-->>P: Return Control instance (via resolveControlName)
P->>C: applies(user, action, model)
C-->>P: Return permission decision (true/false)
P-->>U: Respond with authorization decision
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (13)
src/Perimeters/Perimeter.php (4)
11-11: Clarify or address the TODO.It’s unclear what the “shared example” refers to. Would you like help elaborating on or removing the TODO comment for clarity?
15-18: Placeholder “should(...)” implementation always returns false.This effectively denies all requests. If it’s intentional, consider adding a comment clarifying. Otherwise, consider updating to a more descriptive or dynamic check.
20-26: Validate usage of the stored query callback.Though the design is clear, consider documenting the expected signature of
$queryCallbackto ensure consistent usage across the codebase.
29-36: Consider renaming the static constructor.
public static function new()may conflict with built-in naming expectations. Usingmakeorcreateis a common practice and can improve readability.tests/Support/Access/Controls/ModelControl.php (1)
13-28: Ensure test coverage and consider DRY approach.Each perimeter definition follows a similar pattern. You could consider a shared helper or generator to reduce repetition. Also, confirm that tests adequately cover queries for
is_global,is_client, andis_own.src/Controls/Control.php (5)
19-24: Public static $namespace is mutable.Making
$namespacepublicly mutable could lead to unexpected behavior. Consider restricting write access or introducing an explicit method to change it if needed.
48-62: Factory method for controls.
controlForModelseamlessly delegates toresolveControlName. Consider matching naming conventions (e.g.,makeControlForModel) if you prefer more descriptive factory method names.
63-65: Remove or resolve commented-out TODOs.These comment blocks might be stale or partial. Would you like assistance drafting a follow-up PR to finalize or remove them?
66-74: Static “new()” method naming.Similar to the
Perimeterclass, consider using more conventional naming patterns likecreateormakefor clarity.
100-114: Fallback to'App\\'is a reasonable default.Catching
Throwableensures robust fallback, but consider logging the error for better debugging if the auto-detection fails.config/access-control.php (1)
12-12: Verify the impact of disabling queries by default.This is a breaking change that could affect existing applications. Please ensure this change is documented in the changelog and migration guide.
Consider adding a note in the README or documentation about:
- The reason for this change
- How to enable queries if needed
- Migration steps for existing applications
tests/Feature/PerimetersTest.php (1)
25-25: Enhance test coverage for perimeters.Consider adding tests for:
- Different actions (update, delete, view)
- Edge cases (null user, null model)
- Other perimeters mentioned in the TODO
Would you like me to help generate additional test cases?
src/Controls/HasControl.php (1)
24-29: Add return type hint for control method.The method should have a return type hint for better type safety.
Apply this diff:
- public static function control() + public static function control(): Control
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (40)
.github/ISSUE_TEMPLATE/Bug_report.yml(0 hunks).github/ISSUE_TEMPLATE/Question_or_enhancement_proposal.yml(0 hunks).github/ISSUE_TEMPLATE/config.yml(0 hunks).github/workflows/tests.yml(1 hunks)composer.json(1 hunks)config/access-control.php(1 hunks)src/AccessServiceProvider.php(0 hunks)src/Controls/Concerns/HasPolicy.php(0 hunks)src/Controls/Concerns/HasQuery.php(0 hunks)src/Controls/Control.php(1 hunks)src/Controls/HasControl.php(1 hunks)src/Controls/HasControlScope.php(3 hunks)src/Exceptions/QueryNotImplemented.php(0 hunks)src/Perimeters/Perimeter.php(1 hunks)src/Perimeters/PerimeterCollection.php(0 hunks)src/Perimeters/Perimeters.php(0 hunks)src/PoliciesControlled.php(0 hunks)src/QueriesControlled.php(0 hunks)tests/Feature/PerimetersTest.php(1 hunks)tests/Feature/QueriesTest.php(0 hunks)tests/Feature/TestCase.php(0 hunks)tests/Support/Access/Controls/ModelControl.php(1 hunks)tests/Support/Access/Controls/NotImplementedQueryControl.php(0 hunks)tests/Support/Access/Perimeters/ClientPerimeter.php(1 hunks)tests/Support/Access/Perimeters/GlobalPerimeter.php(1 hunks)tests/Support/Access/Perimeters/OwnPerimeter.php(1 hunks)tests/Support/Access/Perimeters/SharedPerimeter.php(1 hunks)tests/Support/Access/Perimeters/SitePerimeter.php(0 hunks)tests/Support/Access/Policies/ModelPolicy.php(0 hunks)tests/Support/Database/Factories/UserFactory.php(1 hunks)tests/Support/Database/migrations/2014_00_00_000000_create_users_table.php(1 hunks)tests/Support/Database/migrations/2023_04_00_000000_create_models_table.php(1 hunks)tests/Support/Models/Model.php(1 hunks)tests/Support/Models/NotImplementedQueryModel.php(0 hunks)tests/Support/Models/User.php(2 hunks)tests/Support/Traits/InteractsWithAuthorization.php(0 hunks)tests/TestCase.php(1 hunks)tests/Unit/ControlsTest.php(0 hunks)tests/Unit/PoliciesTest.php(0 hunks)tests/Unit/TestCase.php(0 hunks)
💤 Files with no reviewable changes (21)
- tests/Support/Access/Policies/ModelPolicy.php
- tests/Feature/TestCase.php
- tests/Unit/TestCase.php
- .github/ISSUE_TEMPLATE/Question_or_enhancement_proposal.yml
- .github/ISSUE_TEMPLATE/Bug_report.yml
- src/Controls/Concerns/HasQuery.php
- tests/Support/Access/Perimeters/SitePerimeter.php
- src/PoliciesControlled.php
- src/QueriesControlled.php
- src/AccessServiceProvider.php
- src/Controls/Concerns/HasPolicy.php
- .github/ISSUE_TEMPLATE/config.yml
- src/Perimeters/Perimeters.php
- src/Exceptions/QueryNotImplemented.php
- tests/Support/Access/Controls/NotImplementedQueryControl.php
- tests/Unit/PoliciesTest.php
- tests/Feature/QueriesTest.php
- tests/Support/Traits/InteractsWithAuthorization.php
- tests/Support/Models/NotImplementedQueryModel.php
- tests/Unit/ControlsTest.php
- src/Perimeters/PerimeterCollection.php
🧰 Additional context used
🪛 GitHub Actions: tests
composer.json
[error] 1-1: Your requirements could not be resolved to an installable set of packages. The required PHP version (8.2) does not match the current PHP version (8.1.31).
🔇 Additional comments (22)
tests/Support/Access/Perimeters/SharedPerimeter.php (2)
5-7: LGTM! Clean import declarations.The imports are correctly specified and align with the new method signature requirements.
11-14: Consider enhancing test coverage with edge cases.While the implementation is straightforward for testing purposes, consider adding test cases that cover:
- Users with undefined should_shared property
- Null user scenarios
- Different method types
- Various model types
Would you like me to generate additional test cases to improve coverage?
tests/Support/Access/Perimeters/OwnPerimeter.php (2)
5-6: LGTM! Clean import statements.The imports are properly organized and type-safe, using concrete types from Laravel's contracts and Eloquent.
11-14: Consider adding validation for theshould_ownproperty.While the implementation is clean and follows type safety, it assumes the
should_ownproperty always exists on the user model.Let's verify this assumption by checking the user model implementation:
Additionally, consider adding a property check:
public function should(Authenticatable $user, string $method, Model $model): bool { + if (!property_exists($user, 'should_own')) { + return false; + } return $user->should_own; }✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for the definition of should_own property in test user models rg -A 5 "should_own" --type phpLength of output: 1665
Final Response: The "should_own" property is already guaranteed to exist in the User model.
- The search shows that
should_ownis defined in both the test user model (tests/Support/Models/User.php) and set up via the corresponding migration and factory.- As such, adding an extra property check is unnecessary given the codebase's current design.
tests/TestCase.php (1)
92-96: LGTM!The method provides a clean and focused way to assert unauthorized responses, checking both the status code and error message. This helper method will improve test readability and maintainability.
src/Perimeters/Perimeter.php (2)
5-7: Imports look good.These imports appear necessary and relevant; no issues are found.
13-13: Ensure property assignment path is well-defined.
protected Closure $queryCallback;is typed and not nullable. Verify code that uses this property callsquery(...)first, or consider adding a default value to avoid uninitialized property usage.tests/Support/Access/Controls/ModelControl.php (1)
7-9: New perimeter imports.These perimeter imports align with the shift to perimeter-based access control. No issues found.
src/Controls/Control.php (5)
5-6: Imports are appropriate.These dependencies are required for namespace resolution and error handling. No immediate concerns.
Also applies to: 8-8
12-17: Potential concurrency concerns for static property.
static $controlNameResolvercould cause conflicts if modified concurrently at runtime. Confirm if your application flow calls for dynamic changes or if it’s effectively set once in a boot phase.
26-34: Empty perimeters method.This default implementation returning an empty array is presumably a base fallback. Looks fine.
36-46: Naming resolver override method looks good.
guessControlNamesUsingis a clear API to override the control naming mechanism. No further issues noted.
76-98: Namespace string manipulation can be fragile.If a model is not in the default namespace, this logic might fail. Confirm that
$namespaceincludes a trailing backslash and that edge cases for nested namespaces are handled.tests/Support/Access/Perimeters/ClientPerimeter.php (1)
11-14: LGTM! Clean and focused implementation.The
shouldmethod follows a consistent pattern and provides a clear way to check client permissions.tests/Support/Access/Perimeters/GlobalPerimeter.php (1)
11-14: LGTM! Consistent implementation.The
shouldmethod follows the established pattern and provides a clear way to check global permissions.tests/Support/Database/migrations/2023_04_00_000000_create_models_table.php (1)
20-20: LGTM!The column reordering doesn't affect functionality.
tests/Feature/PerimetersTest.php (2)
11-16: LGTM!The test correctly verifies that the client perimeter allows access when
should_clientis true.
18-23: LGTM!The test correctly verifies that the client perimeter denies access when
should_clientis false.tests/Support/Models/User.php (1)
29-32: LGTM! The new access control attributes are properly configured.The new attributes are correctly added to both
$fillableand$castsarrays, ensuring proper mass assignment and boolean type casting.Also applies to: 52-55
tests/Support/Database/Factories/UserFactory.php (1)
34-37: LGTM! The factory definition is properly updated.The new access control attributes are correctly initialized to
falseby default, aligning with the User model changes.src/Controls/HasControlScope.php (1)
3-3: LGTM! The namespace and naming changes improve organization.The changes follow a more consistent naming convention and better code organization.
Also applies to: 9-9, 16-16
.github/workflows/tests.yml (1)
17-17: Verify PHP 8.4 compatibility.PHP 8.4 is currently in development and not yet released. Including it in the test matrix might cause CI failures.
❌ Verification inconclusive
🌐 Web query:
What is the current status of PHP 8.4?Error during web search
Action Required: Adjust PHP Versions in Testing Matrix
- PHP 8.4: Since PHP 8.4 remains in development and isn’t officially released, including it in the test matrix may lead to unexpected CI failures.
- PHP 8.1 & Laravel 11: Laravel 11 requires PHP 8.2 or higher. Retaining PHP 8.1 in the testing matrix might cause compatibility issues.
Please consider revising the test configuration (e.g., removing PHP 8.4 or marking it as an allowed failure, and dropping or updating PHP 8.1) to align with Laravel 11’s requirements.
tests/Support/Database/migrations/2014_00_00_000000_create_users_table.php
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (10)
src/Policies/ControlledPolicy.php (2)
8-8: Consider adding a class docblock
While not strictly necessary, a brief docblock explaining the class purpose would improve clarity and maintainability.
20-20: Use the correct docblock type
The current notationstring-class<Model>is non-standard. Consider usingclass-string<Model>for better accuracy.- * @return string-class<Model> + * @return class-string<Model>tests/Support/Policies/ModelPolicy.php (1)
10-10: Base Eloquent Model
Setting$modeltoModel::classis valid for demonstration; in real usage, referencing a specific model might be preferable.tests/Support/Access/Perimeters/ClientPerimeter.php (1)
10-13: LGTM! Consider adding type hints for better code clarity.The implementation aligns well with the perimeter-based access control approach. The
appliesmethod correctly evaluates the user's client status.Consider adding a type hint for the
should_clientproperty to improve code clarity:- public function applies(Model $user): bool + /** + * @param Model $user User model with bool $should_client property + */ + public function applies(Model $user): booltests/Support/Access/Perimeters/GlobalPerimeter.php (1)
10-13: LGTM! Consider adding type hints for better code clarity.The implementation aligns well with the perimeter-based access control approach. The
appliesmethod correctly evaluates the user's global status.Consider adding a type hint for the
should_globalproperty to improve code clarity:- public function applies(Model $user): bool + /** + * @param Model $user User model with bool $should_global property + */ + public function applies(Model $user): boolsrc/Perimeters/Perimeter.php (2)
12-13: Consider adding property type hints and initialization.The closure properties should be initialized to prevent potential null pointer issues.
Consider initializing the properties with no-op closures:
- protected Closure $queryCallback; - protected Closure $shouldCallback; + protected Closure $queryCallback; + protected Closure $shouldCallback; + + public function __construct() + { + $this->queryCallback = fn(Builder $query) => $query; + $this->shouldCallback = fn(Model $user, string $method, Model $model) => false; + }
15-18: Consider documenting the default behavior.The default implementation of
appliesreturnsfalse, which might not be immediately obvious to implementers.Add documentation to clarify the default behavior:
+ /** + * Determines if this perimeter applies to the given user. + * Default implementation returns false. Override this method in child classes. + */ public function applies(Model $user): bool { return false; }tests/Feature/PerimetersTest.php (1)
14-19: Consider enhancing test coverage with edge cases and complex scenarios.The current tests only verify simple boolean flags. Consider adding tests for:
- Multiple perimeters applied simultaneously
- Edge cases with null/empty values
- Complex scenarios with different combinations of permissions
Also applies to: 21-26, 28-33, 35-40, 42-47, 49-54, 56-61, 63-68
src/Controls/Control.php (1)
33-36: Consider making the empty perimeters method abstract.The empty
perimetersmethod might be confusing. Consider making it abstract to force implementation in child classes.-protected function perimeters(): array +abstract protected function perimeters(): array { - return []; }tests/Feature/ControlsTest.php (1)
10-10: Improve readability by using imports instead of FQCN.The repeated use of fully qualified class names makes the code less readable.
Add import at the top:
+use Lomkit\Access\Tests\Support\Access\Controls\ModelControl; class ControlsTest extends TestCase { public function test_control_with_no_perimeter_passing(): void { - $this->assertFalse((new \Lomkit\Access\Tests\Support\Access\Controls\ModelControl())->applies(Auth::user(), 'create', new Model())); + $this->assertFalse((new ModelControl())->applies(Auth::user(), 'create', new Model())); }Also applies to: 17-17, 28-28, 39-39, 46-46, 57-57, 68-68, 75-75, 86-86, 97-97, 104-104, 115-115, 126-126
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
.github/workflows/tests.yml(1 hunks)composer.json(1 hunks)src/Controls/Control.php(1 hunks)src/Perimeters/Perimeter.php(1 hunks)src/Policies/ControlledPolicy.php(1 hunks)tests/Feature/ControlsTest.php(1 hunks)tests/Feature/PerimetersTest.php(1 hunks)tests/Support/Access/Controls/ModelControl.php(1 hunks)tests/Support/Access/Perimeters/ClientPerimeter.php(1 hunks)tests/Support/Access/Perimeters/GlobalPerimeter.php(1 hunks)tests/Support/Access/Perimeters/OwnPerimeter.php(1 hunks)tests/Support/Access/Perimeters/SharedPerimeter.php(1 hunks)tests/Support/Database/Factories/ModelFactory.php(1 hunks)tests/Support/Database/migrations/2023_04_00_000000_create_models_table.php(1 hunks)tests/Support/Policies/ModelPolicy.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/Support/Access/Perimeters/OwnPerimeter.php
- tests/Support/Database/migrations/2023_04_00_000000_create_models_table.php
- .github/workflows/tests.yml
- composer.json
🔇 Additional comments (19)
tests/Support/Database/Factories/ModelFactory.php (1)
25-31:✅ Verification successful
LGTM! Clean and well-structured factory definition.
The attributes are well-organized with consistent alignment, and the new boolean flags align with the package's reworked access control mechanism.
Let's verify that the database schema matches these factory attributes:
🏁 Script executed:
#!/bin/bash # Description: Check if the database schema matches the factory attributes # Search for the models table migration rg -A 10 "Schema::create.*models" tests/Support/Database/migrations/Length of output: 1407
Factory Definition and Schema Consistency Verified
The factory definition in
tests/Support/Database/Factories/ModelFactory.phpis well-organized and correctly structured. The new boolean flags (is_shared,is_global,is_client,is_own) align perfectly with the corresponding columns in the database migration (tests/Support/Database/migrations/2023_04_00_000000_create_models_table.php). No changes are needed.src/Policies/ControlledPolicy.php (8)
3-3: Namespace change looks good
No issues here; the updated namespace is more consistent with the folder structure.
10-15: Property definition is clear
The$modelproperty and its docblock are consistent, ensuring future maintainers can easily identify its purpose.
22-24: Getter logic aligns with property
Returning the$modelensures consistent usage. Good to see typed return for clarity.
32-34: Control instantiation is valid
Creating aControlwithControl::controlForModel()is straightforward and maintains a clear entry point for policies.
50-50: Consistent usage of Control::should
Good to see a unified approach with$modelin the parameters.
58-58: Consistent usage for create
Mirrors the approach inviewAny(), ensuring consistent permission checks.
66-66: Approach for update is consistent
No concerns with the dynamic call; the pattern remains clear.
74-74: Delete logic follows same pattern
Matches the other methods; no issues noted.tests/Support/Policies/ModelPolicy.php (6)
1-1: No remarks needed
3-3: Namespace usage
Namespace placement is fine and follows PSR-4 structure.
5-5: Importing Model
Straightforward import for referencing Eloquent base class.
6-6: Inherited policy
Cleanly references theControlledPolicybase.
8-8: Extend ControlledPolicy
This extension pattern is clear and straightforward for customizing policy logic.
9-9: No remarks neededtests/Support/Access/Perimeters/SharedPerimeter.php (2)
5-5: Import statement is fine
No issues found with the import ofModel.
10-13:✅ Verification successful
Guard usage of
$user->should_shared
Ensure that theshould_sharedproperty is always set and boolean. Otherwise, calling it could raise issues.
🏁 Script executed:
#!/bin/bash # Look for references to 'should_shared' to confirm it's defined and used consistently. rg 'should_shared' tests srcLength of output: 724
Validation Result:
$user->should_sharedProperty is Consistently Defined as BooleanThe verification confirms that:
- The
should_sharedproperty is defined in the User model with a boolean cast.- The migration file declares
should_sharedas a boolean column.- Test cases consistently update
should_sharedusing boolean values.These checks ensure that its usage in the
applies()method is safe, and no additional guarding is necessary.src/Perimeters/Perimeter.php (1)
10-10: Address the TODO comment.The TODO comment about "shared example" needs clarification or removal.
Please clarify the purpose of this TODO comment and whether it's still relevant.
src/Controls/Control.php (1)
82-83: Address TODO comments before merging.These TODO comments suggest incomplete implementation:
new ClientPerimeter($queryCallback, $policyCallback)shouldCallback déjà définieWould you like me to help implement these features?
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
tests/Feature/ControlsShouldTest.php (1)
6-128: Consider reducing redundancy in test initialization.Each test method directly instantiates
ModelControl. Consider using a setup method or a class property to reduce this duplication.class ControlsShouldTest extends \Lomkit\Access\Tests\Feature\TestCase { + private \Lomkit\Access\Tests\Support\Access\Controls\ModelControl $modelControl; + + protected function setUp(): void + { + parent::setUp(); + $this->modelControl = new \Lomkit\Access\Tests\Support\Access\Controls\ModelControl(); + } + public function test_control_with_no_perimeter_passing(): void { - $this->assertFalse((new \Lomkit\Access\Tests\Support\Access\Controls\ModelControl())->applies(Auth::user(), 'create', new Model())); + $this->assertFalse($this->modelControl->applies(Auth::user(), 'create', new Model())); } // Update the rest of the test methods similarlytests/Feature/ControlsQueryTest.php (2)
39-39: Missing test for overlay perimeter functionality.As noted in the TODO comment, there should be a test for the
overlaysperimeter functionality. Consider implementing this test to ensure complete coverage of the new perimeter system.Would you like me to suggest an implementation for a test that verifies the overlay perimeter functionality?
6-40: Consider reducing redundancy similar to ControlsShouldTest.As with the ControlsShouldTest class, consider refactoring to use a setup method for the ModelControl instantiation to reduce code duplication.
class ControlsQueryTest extends \Lomkit\Access\Tests\Feature\TestCase { + private \Lomkit\Access\Tests\Support\Access\Controls\ModelControl $modelControl; + + protected function setUp(): void + { + parent::setUp(); + $this->modelControl = new \Lomkit\Access\Tests\Support\Access\Controls\ModelControl(); + } + public function test_control_with_no_perimeter_passing(): void { Model::factory() ->count(50) ->create(); $query = Model::query(); - $query = (new \Lomkit\Access\Tests\Support\Access\Controls\ModelControl())->queried($query, Auth::user()); + $query = $this->modelControl->queried($query, Auth::user()); $this->assertEquals(0, $query->count()); }tests/Support/Access/Controls/ModelControl.php (1)
15-20: Handle potential null or empty allowed_methods.In your
$shouldCallback,explode(',', $model->allowed_methods)could behave unexpectedly if$model->allowed_methodsis empty or null. Consider validating or providing a fallback to prevent warnings or unintended empty arrays.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
.github/workflows/tests.yml(1 hunks)composer.json(1 hunks)src/Controls/Control.php(1 hunks)src/Controls/HasControlScope.php(3 hunks)src/Perimeters/OverlayPerimeter.php(1 hunks)src/Perimeters/Perimeter.php(1 hunks)tests/Feature/ControlsQueryTest.php(1 hunks)tests/Feature/ControlsShouldTest.php(1 hunks)tests/Feature/PerimetersTest.php(1 hunks)tests/Support/Access/Controls/ModelControl.php(1 hunks)tests/Support/Access/Perimeters/SharedPerimeter.php(1 hunks)tests/Unit/TestCase.php(0 hunks)
💤 Files with no reviewable changes (1)
- tests/Unit/TestCase.php
🚧 Files skipped from review as they are similar to previous changes (4)
- .github/workflows/tests.yml
- src/Controls/HasControlScope.php
- composer.json
- tests/Feature/PerimetersTest.php
🔇 Additional comments (17)
src/Perimeters/OverlayPerimeter.php (1)
5-11: New OverlayPerimeter class implementation looks correct.This new class creates a specialized perimeter type that always returns true for the
overlays()method. This appears to be part of the larger access control system refactoring mentioned in the PR summary, providing a base class for perimeters that need to overlay permissions.tests/Support/Access/Perimeters/SharedPerimeter.php (3)
5-6: Appropriate import additions.The imports have been correctly updated to support the new class hierarchy and method requirements.
8-8: Class inheritance changed correctly.The inheritance has been appropriately changed from
PerimetertoOverlayPerimeterto align with the refactored access control system.
10-13: Implementation of applies method is clean and correct.The dynamic permission check based on the user's
should_sharedproperty is a good approach, moving from static properties to runtime evaluation.tests/Feature/ControlsShouldTest.php (3)
8-11: Test confirms expected behavior for control without perimeters.This test correctly verifies that a control without any perimeters should not apply.
13-40: Client perimeter tests are comprehensive.The tests for client perimeter thoroughly cover all relevant actions (viewAny, view, create, update, delete) with both positive and negative assertions. The pattern of setting up the user permissions, creating models with specific allowed methods, and testing the outcomes is well-structured.
71-127: Global perimeter tests mirror client tests appropriately.The global perimeter tests follow the same thorough structure as the client perimeter tests, which is good for consistency and comprehensive coverage.
tests/Feature/ControlsQueryTest.php (2)
8-18: Test correctly verifies query behavior without active perimeters.This test effectively confirms that when no perimeter is active for the user, the query returns zero results.
20-37: Client perimeter query test validates expected behavior.The test correctly verifies that when the client perimeter is applied, only the models matching the perimeter criteria are returned in the query.
src/Controls/Control.php (6)
15-20: Add validation for the control name resolver.This static property stores the callback with no checks to ensure it returns an existing control class name. Validating the callback’s return would prevent runtime errors if the callback is misconfigured.
39-50: Confirm the return value for non-existent models.By returning
truewhenever$model->existsis false, you’re implicitly granting access if the model hasn’t been persisted yet. Double-check that this behavior is intentional and won’t open unintended access paths.
56-65: Verify the denial-by-default approach.The query is forced to no results if none of the perimeters apply. Ensure this is correct in scenarios where you might expect a fallback or limited-access result instead of a full denial.
67-70: Approach for zero-result queries looks good.Using
WHERE 0=1is a common pattern for returning an empty result set. The implementation is straightforward and effective.
72-82: Validate the callback in guessControlNamesUsing.Since this method does not validate the callback, there’s a risk of returning an invalid control class name. Validating at assignment would improve reliability.
84-98: Verify existence of resolved control class.
$control = static::resolveControlName($modelName);may fail if the callback or default resolver returns a non-existent class name, resulting in an error on$control::new(). Consider extra checks or a try/catch to handle this gracefully.src/Perimeters/Perimeter.php (2)
16-19: Confirm the default return of true.
appliesalways returnstrue, meaning each perimeter is considered applicable by default. Ensure this is desired or implement additional checks if needed.
21-29: Ensure callbacks are always defined before invocation.If
should(Closure)orquery(Closure)is never invoked,$shouldCallbackor$queryCallbackcould benull, causing runtime errors. Consider default no-op callbacks or protections to avoid unexpected breaks.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/Policies/ControlledPolicy.php (1)
17-17:⚠️ Potential issueInitialize
$modelwith a valid Eloquent model class name.The empty string initialization of
$modelmay cause runtime errors when usingnew ($this->getModel())(lines 44 and 60). This pattern requires$modelto hold a valid Eloquent model class name.This property should be properly overridden in child classes (as seen in the test
ModelPolicyclass) to ensure proper instantiation. Consider either:
- Making this class abstract to force implementation in child classes
- Adding validation in the
getModel()method to throw a descriptive exception- protected string $model = ''; + protected string $model = ''; + + /** + * Get the model class string. + * + * @throws \RuntimeException If the model property is not set in the child class + * @return string-class<Model> + */ + protected function getModel(): string + { + if (empty($this->model)) { + throw new \RuntimeException(sprintf( + 'The $model property must be set on %s.', + static::class + )); + } + + return $this->model; + }src/Controls/Control.php (1)
98-101: 🛠️ Refactor suggestionAdd validation for control name resolver callback.
The
guessControlNamesUsingmethod should validate that the callback returns a valid control class name.Implement validation to ensure the callback returns a valid class name:
public static function guessControlNamesUsing(callable $callback): void { + $testResult = $callback('TestModel'); + if (!is_string($testResult) || !class_exists($testResult)) { + throw new \InvalidArgumentException('Control name resolver must return a valid control class name'); + } static::$controlNameResolver = $callback; }
🧹 Nitpick comments (11)
src/Perimeters/Perimeter.php (4)
11-11: Remove or clarify the TODO comment.
This comment appears to reference an older project structure. Clarifying whether this placeholder is still relevant helps maintain code clarity.
13-16: Prevent potential usage without callbacks.
These closure properties risk runtime errors if used before setting them. Consider providing default callbacks or adding checks to ensure they are set.
33-45: Enhance discoverability with method docblocks.
The chainable methods are well-structured, but adding concise docblocks describing expected callback signatures and return types can improve readability.
59-62: Rename thenew()method for clarity.
Usingnew()as a static named constructor can be confused with the language keyword. Consider using a more descriptive name likecreate()ormake().- public static function new() + public static function create()tests/Support/Access/Perimeters/GlobalPerimeter.php (1)
7-9: Add docblock or usage context.
This class is empty, which might be intentional for test scaffolding. Adding a brief comment or docblock about its intended role will aid maintainability.tests/Feature/ControlsQueryTest.php (1)
20-37: Consider consolidating consecutive user updates.
InvokingAuth::user()->updatemultiple times can be combined to reduce overhead (e.g., a single call updating all relevant fields). This is minor but can streamline test preparation.- Auth::user()->update(['should_client' => true]); ... - Auth::user()->update(['should_shared' => true]); + Auth::user()->update([ + 'should_client' => true, + 'should_shared' => true + ]);src/Controls/Control.php (3)
71-84: Improve error handling in the query control process.The
applyQueryControlmethod doesn't handle potential exceptions from perimeter callbacks, which could lead to query failures in production.Consider adding try-catch blocks to gracefully handle exceptions:
protected function applyQueryControl(Builder $query, Model $user): Builder { foreach ($this->perimeters() as $perimeter) { - if ($perimeter->applyAllowedCallback($user)) { - $query = $perimeter->applyQueryCallback($query, $user); - - if (!$perimeter->overlays()) { - return $query; - } + try { + if ($perimeter->applyAllowedCallback($user)) { + $query = $perimeter->applyQueryCallback($query, $user); + + if (!$perimeter->overlays()) { + return $query; + } + } + } catch (\Throwable $e) { + // Log the error but continue with other perimeters + \Log::error('Error applying perimeter query control', [ + 'perimeter' => get_class($perimeter), + 'error' => $e->getMessage() + ]); } } return $this->noResultQuery($query); }
36-37: Consider allowing customization of empty perimeters behavior.The
perimetersmethod always returns an empty array, which means the default behavior will always deny access unless overridden.Consider providing a configuration option to control the default behavior when no perimeters are defined:
protected function perimeters(): array { return []; } + +/** + * Determine the default response when no perimeters are defined. + * + * @return bool + */ +protected function defaultResponse(): bool +{ + return config('access-control.default_response', false); +}Then update the
appliesmethod:public function applies(Model $user, string $method, Model $model): bool { + $perimeters = $this->perimeters(); + + if (empty($perimeters)) { + return $this->defaultResponse(); + } foreach ($this->perimeters() as $perimeter) { // ... } return false; }
140-151: Improve namespace handling in the resolver.The current implementation of the resolver doesn't handle edge cases like models in subdirectories or non-standard namespace structures well.
Consider improving the namespace handling:
$resolver = static::$controlNameResolver ?? function (string $modelName) { $appNamespace = static::appNamespace(); - $modelName = Str::startsWith($modelName, $appNamespace.'Models\\') - ? Str::after($modelName, $appNamespace.'Models\\') - : Str::after($modelName, $appNamespace); + // Handle models in various namespace structures + $modelBaseName = class_basename($modelName); + + // First check if control exists with full namespace mapping + $fullNamespaceControl = static::$namespace.$modelBaseName.'Control'; + if (class_exists($fullNamespaceControl)) { + return $fullNamespaceControl; + } + + // Fall back to manipulating namespace + $modelName = Str::startsWith($modelName, $appNamespace.'Models\\') + ? Str::after($modelName, $appNamespace.'Models\\') + : Str::after($modelName, $appNamespace); return static::$namespace.$modelName.'Control'; };tests/Support/Access/Controls/ModelControl.php (2)
17-20: Address the TODO comment by extracting the should callback.As mentioned in the TODO comment, the should callback could be extracted to improve code organization.
Extract the callback to a dedicated method:
- // @TODO: possible to extract the should callback to another method ?? - $shouldCallback = function (Model $user, string $method, Model $model) { - return in_array($method, explode(',', $model->allowed_methods)); - }; + return [ + SharedPerimeter::new() + ->should([$this, 'checkAllowedMethods']) + ->allowed(function (Model $user) { + return $user->should_shared; + }) + // ... rest of the code // Add this method to the class + /** + * Check if the method is allowed for the model. + * + * @param Model $user + * @param string $method + * @param Model $model + * @return bool + */ + protected function checkAllowedMethods(Model $user, string $method, Model $model): bool + { + return in_array($method, explode(',', $model->allowed_methods)); + }
22-55: Consider adding type definitions to perimeter callbacks.Type hints in callback functions would improve code safety and IDE support.
Add type hints to all callback functions:
return [ SharedPerimeter::new() ->should($shouldCallback) - ->allowed(function (Model $user) { + ->allowed(function (Model $user): bool { return $user->should_shared; }) - ->query(function (Builder $query, Model $user) { + ->query(function (Builder $query, Model $user): Builder { return $query->orWhere('is_shared', true); }), // ... apply the same pattern to other perimeters
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
config/access-control.php(1 hunks)src/Controls/Control.php(1 hunks)src/Perimeters/Perimeter.php(1 hunks)src/Policies/ControlledPolicy.php(1 hunks)tests/Feature/ControlsQueryTest.php(1 hunks)tests/Feature/PerimetersTest.php(1 hunks)tests/Support/Access/Controls/ModelControl.php(1 hunks)tests/Support/Access/Perimeters/ClientPerimeter.php(0 hunks)tests/Support/Access/Perimeters/GlobalPerimeter.php(1 hunks)tests/Support/Access/Perimeters/OwnPerimeter.php(0 hunks)tests/Support/Access/Perimeters/SharedPerimeter.php(1 hunks)
💤 Files with no reviewable changes (2)
- tests/Support/Access/Perimeters/OwnPerimeter.php
- tests/Support/Access/Perimeters/ClientPerimeter.php
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/Support/Access/Perimeters/SharedPerimeter.php
- tests/Feature/PerimetersTest.php
- config/access-control.php
🔇 Additional comments (7)
src/Perimeters/Perimeter.php (2)
18-26: Confirm exception handling for callbacks.
If the provided closure logic throws exceptions, consider whether you need specific error handling within these “apply” methods to keep the system robust.
67-70: Overlay mechanism is straightforward.
Returningfalseby default is clear. Ensure that overriding perimeters (likeOverlayPerimeter) handle overlay logic adequately to avoid confusion.tests/Feature/ControlsQueryTest.php (3)
34-34: Revisit the TODO comment (duplicate from past reviews).
This French TODO comment was noted in a previous review. Please address it or remove it if no longer applicable.
6-19: Good coverage for the “no perimeter” scenario.
Testing a baseline with zero results is logical. Ensure additional contexts exist or will be added if partial perimeters or fallback logic become relevant.
86-135: Tests are clear and thorough.
These scenarios adequately represent key user attribute combinations. If coverage expands, consider data providers to systematically test various user states.src/Policies/ControlledPolicy.php (1)
44-44: Validate model instantiation.The dynamic instantiation using
new ($this->getModel())assumes the model class has a parameterless constructor, which isn't guaranteed for all Eloquent models.Consider adding a try-catch block to handle potential instantiation errors:
- return $this->getControl()->should($user, __FUNCTION__, new ($this->getModel())); + try { + $model = new ($this->getModel())(); + return $this->getControl()->should($user, __FUNCTION__, $model); + } catch (\Throwable $e) { + \Log::error('Failed to instantiate model for permission check', [ + 'model' => $this->getModel(), + 'error' => $e->getMessage() + ]); + return false; + }tests/Support/Access/Controls/ModelControl.php (1)
28-30:❓ Verification inconclusive
Caution with
orWhereclauses in perimeter queries.Using
orWherein all perimeter queries might create overly permissive queries when multiple perimeters apply.Each perimeter adds an
orWherecondition, which means any record matching any of the conditions will be included. This may not be what you intend if the perimeters should be restrictive rather than additive.Consider:
- Using
whereinstead oforWherefor the first condition- Using subqueries with
->orWhere(function($q) { ... })to group related conditionsAlso applies to: 36-38, 44-46, 52-54
🏁 Script executed:
#!/bin/bash # Find usages of perimeter queries to see expected patterns rg -A 2 "->query\s*\(" --type php src testsLength of output: 450
Action required: Verify Query Patterns and Usage of
orWherein Perimeter QueriesThe current use of
orWherein perimeter queries (e.g., intests/Support/Access/Controls/ModelControl.phpat lines 28-30) could lead to overly permissive results when multiple perimeters apply, since each perimeter simply concatenates anotherorWherecondition. This may inadvertently include records that match just one of the applied perimeters.Consider the following suggestions:
- For the first condition, switching to
wheremight be safer.- Group subsequent conditions using subqueries (e.g., via
->orWhere(function($q) { ... })) to ensure that related conditions are evaluated together.Note on Verification Command:
The previously providedrgcommand failed due to pattern parsing issues. Please run the updated command below to verify the usage of perimeter queries across the codebase:#!/bin/bash rg -A 2 '\-\>query\s*\(' --type php testsThis command should correctly escape the arrow operator. Also, manually verify similar patterns at lines 36-38, 44-46, and 52-54 to ensure that all perimeter queries are implemented as intended.
| //@TODO: what to do for other methods like attach ? It only has view / etc basic methods | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Address the TODO comment.
The TODO comment about handling other methods like "attach" should be addressed before finalizing this PR.
Consider either implementing these additional methods or documenting the decision not to support them. For Laravel's authorization system, you might want to support all standard policy methods.
🏁 Script executed:
#!/bin/bash
# Find all available policy methods in Laravel's authorization system
rg -A 1 "function.*?(?:Policy|Gate)" vendor/laravel --type php | grep "function"Length of output: 135
Action Required: Implement or Document Additional Policy Methods
The file src/Policies/ControlledPolicy.php still contains a TODO regarding methods like attach. Please either add implementations for these missing methods (ensuring consistency with Laravel’s standard policy method set) or update the documentation/comment to explain why they aren’t supported. Note that our initial search for Laravel’s policy methods using the vendor directory (vendor/laravel) did not return results—likely because the expected vendor path isn’t available here—so please verify manually against the Laravel authorization system to confirm the complete set of required methods.
- File:
src/Policies/ControlledPolicy.php - Lines: 10-11
//@TODO: what to do for other methods like attach ? It only has view / etc basic methods
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/Feature/ControlsShouldTest.php (1)
141-151: Test name could be more accurate.The test name
test_control_should_not_delete_global_using_shared_overlayed_perimetersuggests a negative test, but the assertion is checking that deletion is allowed, which is confusing.Rename this test to better reflect that it's checking a positive case with the shared perimeter's specific permission format:
-public function test_control_should_not_delete_global_using_shared_overlayed_perimeter(): void +public function test_control_should_delete_using_shared_specific_permission(): voidtests/Support/Access/Controls/ModelControl.php (1)
17-20: Implement or remove the TODO commentThe TODO comment indicates the intent to extract the should callback to another method, but you've already successfully implemented a callback extraction. Consider removing the TODO comment or actually extracting it to a named method for better organization and reusability.
- // @TODO: possible to extract the should callback to another method ?? - $shouldCallback = function (Model $user, string $method, Model $model) { - return in_array($method, explode(',', $model->allowed_methods)); - }; + // Extract callback to a named method for better organization + $shouldCallback = $this->createMethodPermissionValidator();And add this method to the class:
protected function createMethodPermissionValidator(): \Closure { return function (Model $user, string $method, Model $model) { return in_array($method, explode(',', $model->allowed_methods)); }; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/Controls/Control.php(1 hunks)src/Controls/HasControl.php(1 hunks)src/Perimeters/Perimeter.php(1 hunks)tests/Feature/ControlsQueryTest.php(1 hunks)tests/Feature/ControlsShouldTest.php(1 hunks)tests/Support/Access/Controls/ModelControl.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Controls/HasControl.php
🧰 Additional context used
🧬 Code Definitions (4)
tests/Feature/ControlsShouldTest.php (2)
tests/Support/Access/Controls/ModelControl.php (1)
ModelControl(13-59)src/Controls/Control.php (1)
applies(40-59)
tests/Feature/ControlsQueryTest.php (4)
tests/Unit/TestCase.php (1)
TestCase(6-19)tests/Feature/ControlsShouldTest.php (1)
test_control_with_no_perimeter_passing(8-11)tests/Support/Access/Controls/ModelControl.php (1)
ModelControl(13-59)src/Controls/Control.php (1)
queried(61-74)
src/Controls/Control.php (2)
src/Perimeters/Perimeter.php (8)
Perimeter(9-69)applyAllowedCallback(26-29)should(38-43)applyShouldCallback(16-19)overlays(65-68)query(45-50)applyQueryCallback(21-24)new(57-60)src/Perimeters/OverlayPerimeter.php (1)
overlays(7-10)
src/Perimeters/Perimeter.php (3)
tests/Support/Models/Model.php (1)
Model(10-23)src/Controls/Control.php (1)
new(135-138)src/Perimeters/OverlayPerimeter.php (1)
overlays(7-10)
🔇 Additional comments (39)
tests/Feature/ControlsShouldTest.php (14)
1-11: LGTM - Test for perimeter validation works as expected.The
test_control_with_no_perimeter_passingtest correctly validates that without any perimeter permissions, the control does not apply to any model actions.
13-18: LGTM - Client perimeter viewAny test is well structured.This test correctly validates that when a user has client permissions, they can view any model without checking the model's specific permissions.
20-30: LGTM - Client perimeter view test properly validates permissions.This test accurately verifies that a user with client permissions can view a model when the model's allowed_methods includes 'view'.
31-40: LGTM - Negative test case is properly implemented.This test correctly verifies that permissions are denied when the model's allowed_methods doesn't include the 'view' action.
42-47: LGTM - Client perimeter create test is well structured.The test correctly validates that a user with client permissions can create a model without checking the model's allowed_methods (since the model doesn't exist yet).
49-58: LGTM - Client perimeter update test properly checks permissions.This test correctly validates that a user with client permissions can update a model when the model's allowed_methods includes 'update'.
60-69: LGTM - Client perimeter delete test properly checks permissions.This test correctly validates that a user with client permissions can delete a model when the model's allowed_methods includes 'delete'.
71-76: LGTM - Global perimeter viewAny test is well structured.This test correctly validates that when a user has global permissions, they can view any model without checking the model's specific permissions.
78-87: LGTM - Global perimeter view test properly validates permissions.This test accurately verifies that a user with global permissions can view a model when the model's allowed_methods includes 'view'.
89-98: LGTM - Negative test case for global permissions is properly implemented.This test correctly verifies that global permissions are denied when the model's allowed_methods doesn't include the 'view' action.
100-105: LGTM - Global perimeter create test is well structured.The test correctly validates that a user with global permissions can create a model without checking the model's allowed_methods (since the model doesn't exist yet).
107-116: LGTM - Global perimeter update test properly checks permissions.This test correctly validates that a user with global permissions can update a model when the model's allowed_methods includes 'update'.
118-127: LGTM - Global perimeter delete test properly checks permissions.This test correctly validates that a user with global permissions can delete a model when the model's allowed_methods includes 'delete'.
129-139: LGTM - Overlayed perimeter test is well structured.This test correctly validates the behavior when both shared and global permissions are present, checking that the standard delete method works as expected.
tests/Feature/ControlsQueryTest.php (7)
8-18: LGTM - Test for query with no perimeter permissions is well structured.This test correctly validates that without any perimeter permissions, querying returns no records.
20-36: LGTM - Client perimeter query test is properly implemented.This test correctly validates that when a user has client permissions, querying only returns records with the is_client flag set to true.
38-59: LGTM - Overlayed perimeter test checks correct record count.This test verifies that when shared and client permissions are both active, the query returns records matching either is_shared or is_client flags (100 records total).
61-83: LGTM - Overlayed perimeter with distant perimeter test is properly implemented.This test validates that when shared and own permissions are active, the queried method correctly returns both is_shared and is_own records (100 total).
85-106: LGTM - Test for only shared overlay perimeter is well structured.This test correctly verifies that when only shared permissions are active, only is_shared records are returned (50 total).
108-130: LGTM - Isolated query test verifies correct behavior.This test properly checks that when queries are isolated (default behavior) and a base query specifies is_client=true, only the shared records that are also client records are returned.
132-156: LGTM - Non-isolated query test verifies correct behavior.This test properly checks that when queries are not isolated and a base query specifies is_client=true, all records (including ones that don't match the client filter) are returned.
src/Controls/Control.php (11)
15-28: LGTM - Static properties for control resolution are well defined.The static properties for control name resolution and namespace are properly defined with clear docblocks.
30-38: LGTM - Simplified perimeters method returns empty array by default.This change allows extending classes to override the method to provide custom perimeters.
40-59: LGTM - The applies method is well implemented.The method properly:
- Iterates through perimeters
- Checks if perimeter allows the user
- Fast-returns true for non-existent models (e.g., during 'create' operations)
- Applies the should callback to determine specific permissions
- Handles overlay perimeters correctly
61-74: LGTM - Queried method properly handles isolated queries.The method correctly implements isolation based on configuration and delegates to applyQueryControl for the actual query modification.
76-95: LGTM - Query control application logic is well implemented.The method properly handles:
- Setting up a default no-result callback
- Iterating through perimeters
- Applying query callbacks for allowed perimeters
- Handling overlay perimeters correctly
This implementation ensures that query results are properly filtered based on user permissions.
97-100: LGTM - No result query helper is simple and effective.The method appropriately uses a raw SQL condition that always evaluates to false to ensure no results are returned.
102-112: Add validation for the control name resolver callback.The
guessControlNamesUsingmethod should validate that the provided callback returns a valid control class name to prevent runtime errors.public static function guessControlNamesUsing(callable $callback): void { + $testResult = $callback('TestModel'); + if (!is_string($testResult) || !class_exists($testResult)) { + throw new \InvalidArgumentException('Control name resolver must return a valid control class name'); + } static::$controlNameResolver = $callback; }
114-128: LGTM - Control for model factory method is well implemented.The static factory method properly resolves the control name and creates a new instance of the appropriate control class.
130-138: LGTM - Static factory method for instantiation is properly implemented.The static
new()method follows the factory pattern consistently used throughout the codebase.
140-162: LGTM - Control name resolution logic is comprehensive.The
resolveControlNamemethod properly:
- Uses the custom resolver if provided
- Falls back to a default resolver that:
- Gets the application namespace
- Handles models within or outside the Models namespace
- Constructs the control name with the proper namespace
164-178: LGTM - Application namespace resolution is properly implemented with error handling.The method safely gets the application namespace and falls back to 'App\' if any exceptions occur.
src/Perimeters/Perimeter.php (5)
11-14: LGTM - Protected callback properties are properly defined.The properties for storing callback closures are properly defined with type declarations.
16-29: LGTM - Callback application methods are well implemented.These methods properly:
- Execute the stored callback closures
- Pass the appropriate parameters
- Return the expected values
31-50: LGTM - Callback setter methods enable fluent interface.The methods for setting callback closures:
- Store the provided callback
- Return
$thisto enable method chaining- Follow a consistent pattern
52-60: LGTM - Static factory method is properly implemented.The static
new()method follows the factory pattern consistently used throughout the codebase.
62-68: LGTM - Overlays method defines default overlay behavior.The
overlays()method returns false by default, indicating that perimeters don't overlay unless specifically designed to do so (like inOverlayPerimeter).tests/Support/Access/Controls/ModelControl.php (2)
27-29: Inconsistent logic in SharedPerimeter's should callbackThe
SharedPerimeteruses a different validation logic ($method.'_shared') compared to other perimeters which use the extracted$shouldCallback. Either document why this inconsistency exists or consider unifying the approach.Does the
SharedPerimeterspecifically need to check for method names with the "_shared" suffix? If not, consider using the same callback for consistency:->should(function (Model $user, string $method, Model $model) { - return in_array($method.'_shared', explode(',', $model->allowed_methods)); + return in_array($method, explode(',', $model->allowed_methods)); })
15-57: Improvements implemented per previous review commentsThe changes incorporate many improvements suggested in previous reviews:
- The common should callback has been extracted to reduce duplication
in_arraywithexplodeis now used for more reliable method validation- All query callbacks properly return the updated query
Good job implementing these improvements.
| ->query(function (Builder $query, Model $user) { | ||
| return $query->orWhere('is_shared', true); | ||
| }), |
There was a problem hiding this comment.
Potential query logic issue with orWhere conditions
All perimeters use orWhere conditions which can lead to unexpected results when combined with other query constraints. Using orWhere makes each condition an alternative path rather than an additional filter.
If these conditions should be alternatives (OR logic), consider using a query scope or wrapping them in a where closure. If they should be additive (AND logic), use where instead of orWhere:
->query(function (Builder $query, Model $user) {
- return $query->orWhere('is_shared', true);
+ return $query->where('is_shared', true);
}),If OR logic is truly intended, use query scopes or where closures to isolate the OR conditions:
->query(function (Builder $query, Model $user) {
- return $query->orWhere('is_shared', true);
+ return $query->where(function($q) {
+ $q->orWhere('is_shared', true);
+ });
}),Also applies to: 38-40, 46-48, 54-56
There was a problem hiding this comment.
@coderabbitai I added many changes to this PR please do it from the beginning
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Get the latest PR details
gh pr view 5 --json files,title,body
# Check the current implementation of ModelControl.php
cat tests/Support/Access/Controls/ModelControl.php
# Check the Control base class implementation
cat src/Controls/Control.php
# Check perimeter implementations
cat src/Perimeters/Perimeter.php
cat tests/Support/Access/Perimeters/SharedPerimeter.php
cat tests/Support/Access/Perimeters/ClientPerimeter.php
cat tests/Support/Access/Perimeters/GlobalPerimeter.php
cat tests/Support/Access/Perimeters/OwnPerimeter.php
# Check if there are any tests that verify the querying behavior
ls -la tests/Feature/
cat tests/Feature/ControlsQueryTest.php 2>/dev/null || echo "File not found"
Length of output: 19255
@GautierDele I'll start my review fresh as you requested. Looking at the current code, I can see that your PR includes a complete rework of the package with a new perimeter-based approach.
The current implementation in ModelControl uses orWhere conditions in all perimeter query callbacks, but I see this is intentional by design and is properly handled in the Control class. The queried() method in the base Control class automatically wraps these conditions in a where closure when the access-control.queries.isolated config is true (which is the default behavior).
Your tests in ControlsQueryTest.php clearly demonstrate this works correctly, with separate test cases for both isolated and non-isolated queries. The OR logic between perimeters (especially with the SharedPerimeter that extends OverlayPerimeter) is well thought out and thoroughly tested.
This architecture is much more flexible and maintainable than the previous implementation. Are there any specific aspects of the new design you'd like me to focus on in my review?
🧬 Code Definitions Used
Relevant Code Snippets
1. OwnPerimeter Class
- Filename:
tests/Support/Access/Perimeters/OwnPerimeter.php - Line Range: 7-9
class OwnPerimeter extends Perimeter
{
}- Description: This class extends the
Perimeterclass, which is likely used in theModelControlclass to define specific access control logic.
2. ClientPerimeter Class
- Filename:
tests/Support/Access/Perimeters/ClientPerimeter.php - Line Range: 7-9
class ClientPerimeter extends Perimeter
{
}- Description: This class extends the
Perimeterclass, similar toOwnPerimeter, and is utilized in theModelControlclass for access control.
3. GlobalPerimeter Class
- Filename:
tests/Support/Access/Perimeters/GlobalPerimeter.php - Line Range: 7-9
class GlobalPerimeter extends Perimeter
{
}- Description: This class also extends the
Perimeterclass and is part of the access control mechanisms in theModelControlclass.
4. SharedPerimeter Class
- Filename:
tests/Support/Access/Perimeters/SharedPerimeter.php - Line Range: 7-9
class SharedPerimeter extends OverlayPerimeter
{
}- Description: This class extends
OverlayPerimeterand is used in theModelControlclass to manage shared access control.
5. Control Class
- Filename:
src/Controls/Control.php - Line Range: 13-179
class Control
{
// @TODO: scout queried
/**
* The control name resolver.
*
* @var callable
*/
protected static $controlNameResolver;
/**
* The default namespace where control reside.
*
* @var string
*/
public static $namespace = 'App\\Access\\Controls\\';
/**
* Get the perimeters for the current control.
*
* @return array<Perimeter>
*/
protected function perimeters(): array
{
return [];
}
public function applies(Model $user, string $method, Model $model): bool
{
foreach ($this->perimeters() as $perimeter) {
if ($perimeter->applyAllowedCallback($user)) {
// If the model doesn't exists, it means the method is not related to a model
// so we don't need to activate the should result since we can't compare an existing model
if (!$model->exists) {
return true;
}
$should = $perimeter->applyShouldCallback($user, $method, $model);
if (!$perimeter->overlays() || $should) {
return $should;
}
}
}
return false;
}
public function queried(Builder $query, Model $user): Builder
{
$callback = function (Builder $query, Model $user) {
return $this->applyQueryControl($query, $user);
};
if (config('access-control.queries.isolated')) {
return $query->where(function (Builder $query) use ($user, $callback) {
$callback($query, $user);
});
}
return $callback($query, $user);
}
protected function applyQueryControl(Builder $query, Model $user): Builder
{
$noResultCallback = function (Builder $query) {
return $this->noResultQuery($query);
};
foreach ($this->perimeters() as $perimeter) {
if ($perimeter->applyAllowedCallback($user)) {
$query = $perimeter->applyQueryCallback($query, $user);
$noResultCallback = function ($query) {return $query; };
if (!$perimeter->overlays()) {
return $query;
}
}
}
return $noResultCallback($query);
}
protected function noResultQuery(Builder $query): Builder
{
return $query->whereRaw('0=1');
}
/**
* Specify the callback that should be invoked to guess control names.
*
* @param callable(class-string<\Illuminate\Database\Eloquent\Model>): class-string<\Lomkit\Access\Controls\Control> $callback
*
* @return void
*/
public static function guessControlNamesUsing(callable $callback): void
{
static::$controlNameResolver = $callback;
}
/**
* Get a new control instance for the given model name.
*
* @template TClass of \Illuminate\Database\Eloquent\Model
*
* @param class-string<TClass> $modelName
*
* @return \Lomkit\Access\Controls\Control<TClass>
*/
public static function controlForModel(string $modelName): self
{
$control = static::resolveControlName($modelName);
return $control::new();
}
/**
* Get a new control instance for the given attributes.
*
* @return static
*/
public static function new(): self
{
return new static();
}
/**
* Get the control name for the given model name.
*
* @template TClass of \Illuminate\Database\Eloquent\Model
*
* @param class-string<TClass> $modelName
*
* @return class-string<\Lomkit\Access\Controls\Control<TClass>>
*/
public static function resolveControlName(string $modelName): string
{
$resolver = static::$controlNameResolver ?? function (string $modelName) {
$appNamespace = static::appNamespace();
$modelName = Str::startsWith($modelName, $appNamespace.'Models\\')
? Str::after($modelName, $appNamespace.'Models\\')
: Str::after($modelName, $appNamespace);
return static::$namespace.$modelName.'Control';
};
return $resolver($modelName);
}
/**
* Get the application namespace for the application.
*
* @return string
*/
protected static function appNamespace(): string
{
try {
return Container::getInstance()
->make(Application::class)
->getNamespace();
} catch (Throwable) {
return 'App\\';
}
}
}- Description: This class provides the base functionality for access control, including methods for applying perimeters and querying based on user permissions.
6. perimeters Method in Control Class
- Filename:
src/Controls/Control.php - Line Range: 35-38
protected function perimeters(): array
{
return [];
}- Description: This method is intended to return an array of perimeters for the control, which is overridden in the
ModelControlclass to provide specific access control logic.
7. new Method in Control Class
- Filename:
src/Controls/Control.php - Line Range: 135-138
public static function new(): self
{
return new static();
}- Description: This static method creates a new instance of the control class, which is used in the
ModelControlclass for instantiation.
These snippets provide context and functionality related to the ModelControl class and its access control mechanisms.
Summary by CodeRabbit