Feature/refactored #13
Conversation
WalkthroughThis update introduces significant changes to the access control system and its supporting test infrastructure. The perimeter and control logic is refactored to use permission-based checks and explicit Eloquent relationships instead of boolean flags. The test suite and database schema are restructured to align with these changes, introducing new factories, migrations, and relationships for Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Gate
participant Control
participant Perimeter
participant Model
User->>Gate: can('view client models')
Gate-->>User: true/false
User->>Control: applies(user, method, model)
Control->>Perimeter: applyAllowedCallback(user, method)
Perimeter-->>Control: permission result
Control->>Perimeter: applyShouldCallback(user, model)
Perimeter-->>Control: model match result
Control-->>User: access allowed/denied
User->>Model: query (with Control filters)
Model->>Control: filter query by perimeter (e.g., client_id, author_id, sharedWithUsers)
Control-->>Model: filtered query
Possibly related PRs
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
🪧 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: 6
🧹 Nitpick comments (10)
.github/FUNDING.yml (1)
1-1: Add a newline at end of file for formatting consistency.The FUNDING.yml content is correct. For best practices and to satisfy some linters, add a newline at the end of the file.
-github: GautierDele +github: GautierDele +🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: no new line character at the end of file
(new-line-at-end-of-file)
.github/workflows/packagist-deploy.yml (1)
1-20: Add a newline at end of file for formatting consistency.The workflow is well-structured and secure. For best practices and to satisfy some linters, add a newline at the end of the file.
- api_token: ${{ secrets.PACKAGIST_TOKEN }} + api_token: ${{ secrets.PACKAGIST_TOKEN }} +🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 20-20: no new line character at the end of file
(new-line-at-end-of-file)
tests/Support/Database/migrations/2013_04_00_000000_create_clients_table.php (1)
15-19: Consider adding a unique constraint to the client name.The
clientstable looks well-structured. Since this appears to be part of a multi-tenant architecture, you might want to consider making the client name unique to prevent duplicate client entries.Schema::create('clients', function (Blueprint $table) { $table->id(); - $table->string('name'); + $table->string('name')->unique(); $table->timestamps(); });tests/Feature/TestCase.php (1)
11-19: Consider adding helper methods for different user-client scenarios.While the current setup creates a standard user with a client for all tests, you might want to consider adding helper methods for creating users with specific client relationships to support more targeted testing scenarios.
/** * Create a user with a specific client * * @param Client|null $client * @return User */ protected function createUserWithClient(?Client $client = null): User { return UserFactory::new() ->for($client ?? Client::factory()) ->createOne(); } /** * Create and authenticate a user with a specific client * * @param Client|null $client * @return $this */ protected function withAuthenticatedUserForClient(?Client $client = null) { $user = $this->createUserWithClient($client); return $this->withAuthenticatedUser($user); }tests/Support/Database/migrations/2014_00_00_000000_create_users_table.php (1)
21-21: Consider adding an index to the client_id column.Since you'll likely query users by client frequently, adding an index to the client_id column could improve query performance.
-$table->foreignIdFor(\Lomkit\Access\Tests\Support\Models\Client::class, 'client_id')->nullable()->constrained(); +$table->foreignIdFor(\Lomkit\Access\Tests\Support\Models\Client::class, 'client_id')->nullable()->constrained()->index();tests/Support/Database/Factories/ClientFactory.php (1)
10-11: Incorrect PHPDoc type specificationThe PHPDoc comment indicates this factory extends Factory for the
Usermodel, but the actual class creates instances of theClientmodel.-/** - * @extends \Illuminate\Database\Eloquent\Factories\Factory<User> - */ +/** + * @extends \Illuminate\Database\Eloquent\Factories\Factory<Client> + */tests/Feature/PerimetersTest.php (1)
14-19: Consider adding more comprehensive perimeter testsWhile the simplification of these tests makes sense with the refactoring, consider adding more comprehensive tests that verify the actual relationships and permission checks that would occur in production code.
Some examples:
- Test that a client perimeter only allows access to models belonging to the user's client
- Test that a shared perimeter allows access only to models explicitly shared with the user
- Test the interaction between different perimeters in realistic scenarios
README.md (1)
55-55: Grammar correction needed.The word "setup" is a noun. For the verb form, use "set up" with a space.
-Then setup your policy: +Then set up your policy:🧰 Tools
🪛 LanguageTool
[grammar] ~55-~55: The word “setup” is a noun. The verb is spelled with a space.
Context: ... }), // ...Then setup your policy:php class PostPoli...(NOUN_VERB_CONFUSION)
tests/Feature/ControlsQueryTest.php (2)
26-28: Always returningtruefor 'view client models'
This is fine for testing but be aware that it enables unrestricted access. If you want to test permission failures, consider adding more realistic checks.
126-131: Gates for 'view shared models' and 'view own models'
Again, returningtrueenables universal access. This may be intentional for test coverage, but keep in mind it doesn’t test permission denial.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
.github/FUNDING.yml(1 hunks).github/workflows/packagist-deploy.yml(1 hunks)README.md(1 hunks)src/Controls/Control.php(3 hunks)src/Perimeters/Perimeter.php(3 hunks)tests/Feature/ControlsQueryTest.php(7 hunks)tests/Feature/ControlsScoutQueryTest.php(2 hunks)tests/Feature/ControlsShouldTest.php(2 hunks)tests/Feature/PerimetersTest.php(1 hunks)tests/Feature/TestCase.php(2 hunks)tests/Support/Access/Controls/ModelControl.php(1 hunks)tests/Support/Database/Factories/ClientFactory.php(1 hunks)tests/Support/Database/Factories/ModelFactory.php(2 hunks)tests/Support/Database/Factories/UserFactory.php(0 hunks)tests/Support/Database/migrations/2013_04_00_000000_create_clients_table.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/Database/migrations/2023_05_00_000000_create_models_shared_with_users_table.php(1 hunks)tests/Support/Models/Client.php(1 hunks)tests/Support/Models/Model.php(1 hunks)tests/Support/Models/User.php(1 hunks)
💤 Files with no reviewable changes (1)
- tests/Support/Database/Factories/UserFactory.php
🧰 Additional context used
🧬 Code Graph Analysis (9)
tests/Feature/TestCase.php (3)
tests/Support/Models/Client.php (1)
Client(9-36)tests/Support/Database/Factories/UserFactory.php (1)
UserFactory(12-41)tests/TestCase.php (1)
withAuthenticatedUser(82-85)
tests/Support/Models/Model.php (2)
tests/Support/Models/User.php (2)
User(10-64)client(50-53)tests/Support/Models/Client.php (1)
Client(9-36)
src/Controls/Control.php (1)
src/Perimeters/Perimeter.php (3)
applyAllowedCallback(76-79)should(102-107)applyShouldCallback(38-41)
tests/Feature/ControlsShouldTest.php (5)
tests/Support/Models/Model.php (1)
Model(11-40)tests/Support/Models/User.php (1)
User(10-64)tests/Support/Access/Controls/ModelControl.php (1)
ModelControl(13-74)src/Controls/Control.php (1)
applies(49-68)tests/Support/Database/Factories/ModelFactory.php (1)
clientPerimeter(26-29)
tests/Support/Database/migrations/2014_00_00_000000_create_users_table.php (1)
tests/Support/Models/Client.php (1)
Client(9-36)
tests/Support/Database/migrations/2023_04_00_000000_create_models_table.php (2)
tests/Support/Models/User.php (1)
User(10-64)tests/Support/Models/Client.php (1)
Client(9-36)
tests/Support/Database/Factories/ModelFactory.php (2)
tests/Support/Models/Model.php (3)
client(31-34)Model(11-40)sharedWithUsers(36-39)tests/Support/Models/User.php (1)
client(50-53)
tests/Support/Database/migrations/2023_05_00_000000_create_models_shared_with_users_table.php (4)
tests/Support/Database/migrations/2023_04_00_000000_create_models_table.php (1)
up(13-25)tests/Support/Database/migrations/2014_00_00_000000_create_users_table.php (1)
up(13-25)tests/Support/Models/Model.php (1)
Model(11-40)tests/Support/Models/User.php (1)
User(10-64)
tests/Feature/PerimetersTest.php (4)
tests/Support/Access/Perimeters/ClientPerimeter.php (1)
ClientPerimeter(7-9)src/Perimeters/Perimeter.php (2)
allowed(88-93)applyAllowedCallback(76-79)tests/Support/Models/Model.php (1)
Model(11-40)tests/Support/Access/Perimeters/SharedPerimeter.php (1)
SharedPerimeter(7-9)
🪛 YAMLlint (1.35.1)
.github/FUNDING.yml
[error] 1-1: no new line character at the end of file
(new-line-at-end-of-file)
.github/workflows/packagist-deploy.yml
[error] 20-20: no new line character at the end of file
(new-line-at-end-of-file)
🪛 LanguageTool
README.md
[grammar] ~55-~55: The word “setup” is a noun. The verb is spelled with a space.
Context: ... }), // ... Then setup your policy: php class PostPoli...
(NOUN_VERB_CONFUSION)
🔇 Additional comments (46)
tests/Support/Database/migrations/2013_04_00_000000_create_clients_table.php (1)
7-21: LGTM: Migration structure follows best practices.The migration follows Laravel best practices with proper class structure, docblocks, and naming conventions. The file is properly ordered (2013_* timestamp) to run before user migrations that will reference this table.
tests/Feature/TestCase.php (2)
6-6: LGTM: Adding Client model import for relationship.Properly adds the import for the Client model needed for the factory relationship in setUp.
15-18: LGTM: TestCase now creates users with client association.The updated setUp method now creates users with an associated client, which aligns well with the refactored permission-based access control system. This ensures all tests run with properly related entities.
tests/Support/Database/migrations/2023_05_00_000000_create_models_shared_with_users_table.php (1)
7-21: LGTM: Pivot table migration supports many-to-many relationships.The migration properly creates a pivot table with appropriate foreign keys and timestamps, supporting the many-to-many relationship between models and users for the model sharing functionality.
tests/Support/Database/migrations/2014_00_00_000000_create_users_table.php (1)
21-21: LGTM: Foreign key replaces boolean flags for access control.Adding the
client_idforeign key is a good refactoring approach, replacing the boolean flags previously used for access control with a relationship-based approach. This provides more flexibility and maintainability.tests/Support/Database/Factories/ClientFactory.php (1)
1-27: Well-structured factory implementationThe
ClientFactoryimplementation follows Laravel's factory pattern correctly, with proper namespace, class extension, model specification, and a clean definition method.tests/Support/Models/Client.php (1)
9-36: Well-implemented Client model with appropriate relationshipsThe Client model is properly structured with all necessary components:
- Extends Authenticatable for potential auth functionality
- Uses HasFactory trait
- Properly implements newFactory()
- Defines appropriate fillable attributes
- Establishes correct relationships with Model and User entities
This implementation aligns well with the broader refactoring toward relationship-based access control.
tests/Support/Models/Model.php (1)
26-39: Good relationship structure for access controlThe newly added relationship methods (
author(),client(), andsharedWithUsers()) are well-implemented and follow Laravel conventions. These relationships establish the foundation for the permission-based access control system, replacing the previous boolean flags approach with a more flexible and explicit model.tests/Feature/PerimetersTest.php (2)
14-14: Simplified perimeter test with updated callback signatureThe test now uses the updated perimeter callback signature that includes a
$methodparameter, aligning with changes in the perimeter implementation. The test is simplified to use a fixed boolean return.
19-19: Simplified perimeter test with updated callback signatureThe SharedPerimeter test also adopts the updated perimeter callback signature with the
$methodparameter, consistent with the ClientPerimeter test and the overall refactoring approach.README.md (3)
5-13: LGTM! Clear and informative introduction.The introduction now clearly communicates the package's purpose, requirements, and provides links to comprehensive documentation, making it much easier for users to get started.
16-53: Well-structured example that demonstrates the permission-based approach.The code examples effectively illustrate how to define perimeters with permission checks (
$user->can()) and relationship-based logic ($model->client()->is($user->client)). This aligns perfectly with the refactored design that uses Laravel Gates and Eloquent relationships instead of boolean flags.
57-70: LGTM! Clear usage examples for policy implementation and query execution.The policy setup and usage examples clearly demonstrate how to extend
ControlledPolicyand how to use the controlled queries and authorization checks.tests/Support/Models/User.php (1)
49-63: LGTM! Well-structured Eloquent relationships that replace boolean flags.The refactoring from boolean flags to explicit Eloquent relationships is a significant improvement:
client()- Creates a belongs-to relationship with the Client modelmodels()- Establishes a has-many relationship for models authored by this usersharedModels()- Implements a many-to-many relationship for shared modelsThis approach provides better encapsulation, type safety, and follows Laravel's conventions for model relationships.
tests/Support/Database/migrations/2023_04_00_000000_create_models_table.php (1)
21-22: LGTM! Improved database schema with proper relationships.Replacing boolean flags with foreign key relationships is a significant improvement that:
- Enables proper relational integrity through foreign key constraints
- Supports the relationship-driven access control approach
- Makes queries more efficient by leveraging database indexes on foreign keys
The
author_idandclient_idfields align perfectly with the relationships defined in the User model.src/Controls/Control.php (4)
52-52: LGTM! Updated allowed callback to include method parameter.The method parameter is now correctly passed to the
applyAllowedCallbackmethod, allowing for dynamic permission checks based on the specific action being performed.
59-59: LGTM! Updated should callback to remove method parameter.The method parameter has been removed from the
applyShouldCallbackcall, aligning with the updated signature in the Perimeter class that now focuses solely on the user-model relationship.
121-121: LGTM! Consistent permission check for view action in query control.The 'view' action is now explicitly passed to the
applyAllowedCallbackmethod, making the permission check consistent with the overall design that uses method-specific permissions.
150-150: LGTM! Consistent permission check for view action in Scout query control.Similar to the standard query control, the Scout query control now correctly passes the 'view' action to the
applyAllowedCallbackmethod, ensuring consistent permission checking across different query types.tests/Support/Database/Factories/ModelFactory.php (4)
6-6: Added Auth facade import to support perimeter factory states.This import is necessary to access the authenticated user data in the newly added perimeter-specific factory state methods.
26-29: Well-implemented client perimeter factory state.The
clientPerimeter()method properly associates models with the authenticated user's client using thefor()factory method. This elegantly replaces the previous booleanis_clientapproach with an actual relationship.
31-36: Good use of afterCreating to handle the many-to-many relationship.The
sharedPerimeter()method correctly uses theafterCreatinghook to establish the many-to-many relationship with thesharedWithUserspivot table. This approach is appropriate since many-to-many relationships typically need to be established after the model is created.
38-41: Clean implementation of author relationship.The
ownPerimeter()method correctly associates the model with the authenticated user as its author using thefor()method with a named relationship.tests/Feature/ControlsShouldTest.php (4)
6-8: Appropriate imports for the new authorization approach.The added imports support the shift from attribute-based to gate-based authorization, which is a more standard Laravel approach.
19-21: Effective shift to Gate-based authorization.Replacing direct user attribute updates with Laravel Gate definitions is a more maintainable and flexible approach to authorization.
28-34: Good combination of Gate definition and updated factory usage.The test properly defines a gate for client models viewing and uses the new
clientPerimeter()factory state to create a test model with the appropriate relationship, ensuring the test accurately reflects the new relationship-based access control system.
154-162: Well-structured multi-gate test for overlayed perimeter.This test correctly defines multiple gates to test the overlaying behavior between shared and global perimeters, which is an important aspect of the access control system.
tests/Feature/ControlsScoutQueryTest.php (4)
4-6: Proper imports for Gate-based authorization in Scout queries.The added imports align with the shift to gate-based authorization throughout the codebase.
20-27: Updated Scout query test with appropriate gate definition.The test correctly defines a gate for viewing client models and asserts that the Scout query includes the client_id filter. The assertion has been properly updated to check for the relationship-based filter instead of a boolean flag.
32-42: Comprehensive test for overlayed perimeters in Scout queries.The test properly defines gates for both client and shared models, and correctly verifies that both filters are applied to the Scout query. The updated assertion checks for relationship-based filters rather than boolean flags.
47-57: Good test coverage for distant perimeter relationships.The test properly defines gates for own and shared models and verifies that the Scout query correctly includes both author_id and shared_with_users filters.
src/Perimeters/Perimeter.php (3)
25-26: Updated callback signatures for better permission control.The default callbacks have been updated to better reflect their responsibilities:
shouldCallbackno longer needs the method parameter since it only checks if a model falls within a perimeterallowedCallbacknow explicitly requires the method parameter for fine-grained permission checkingThis change better separates the concerns of permission checking and perimeter applicability.
38-41: Simplified shouldCallback implementation.Removing the method parameter from
applyShouldCallbackaligns with the updated callback signature and clarifies that this check is only about whether a model belongs to a perimeter, not about permission to perform a specific action.
76-79: Enhanced allowedCallback with method parameter.Adding the method parameter to
applyAllowedCallbackenables more fine-grained permission checks based on the specific action being performed, which is a key aspect of the new permission-based access control system.tests/Feature/ControlsQueryTest.php (9)
6-6: New Gate import looks correct
No issues: the import is necessary for gate definitions below.
8-8: User model import
This allows referencing Gate definitions with theUserclass correctly.
46-51: Permissive gates for 'view client models' and 'view shared models'
Defining both gates to returntrueallows broad access in tests. If coverage of permission failures is needed, you might want additional condition checks.
73-78: Permissive gates for 'view shared models' and 'view own models'
Returningtruegrants unlimited access. Acceptable for broad test coverage but doesn't simulate denied scenarios.
101-103: Gate definition for 'view shared models' returningtrue
No immediate concerns; fits the pattern of permissive test gates.
106-106: Utilizing new perimeter factory states
Using->clientPerimeter(),->sharedPerimeter(), and->ownPerimeter()clarifies test setup and aligns with the relationship-based model.Also applies to: 110-110, 114-114
134-134: Client perimeter plus additional states and explicitwherecondition
Chaining multiple perimeter states is consistent with the new approach. EnsureAuth::user()->clientcannot be null in these tests—otherwise this query can fail.Also applies to: 138-139, 143-143, 147-147
157-162: Defining gates for 'view shared models' and 'view own models'
Returningtrueis typical for test stubs, but consider variations if testing different permission levels.
165-165: Chaining perimeter states and narrowing the query by client
No major issues; just ensureAuth::user()->clientis guaranteed to be set for this scenario to avoid null references.Also applies to: 169-170, 174-174, 178-178
tests/Support/Access/Controls/ModelControl.php (3)
19-20: SharedPerimeter logic
- The
allowedcallback delegates toGate::can(...)for'shared models'.- The
shouldcallback verifies the model is actually shared with the user, which is correct.- The
scoutQueryuses'shared_with_users'; confirm your Scout indexing supports this field.- The
queryusesorWhereHas('sharedWithUsers')to retrieve shared models, which aligns with the intended logic.Also applies to: 22-23, 26-26, 29-31
34-36: GlobalPerimeter logic
- The
allowedcallback correctly checks'global models'permissions.- The
shouldcallback always returnstrue, meaning a fully open perimeter.- The
scoutQueryandqueryapply no restrictions, consistent with global access.Also applies to: 37-38, 41-41, 44-44
47-49: ClientPerimeter logic
- The
allowedcallback checks'client models'viaGate::can(...).- The
shouldcallback enforcesmodel->client()matchinguser->client.- The queries filter by
client_id. Verifyuser->clientis always defined to avoid null references.Also applies to: 50-51, 54-54, 57-57
tests/Support/Database/migrations/2013_04_00_000000_create_clients_table.php
Outdated
Show resolved
Hide resolved
tests/Support/Database/migrations/2023_05_00_000000_create_models_shared_with_users_table.php
Show resolved
Hide resolved
| Gate::define('view client models', function (User $user) { | ||
| return true; | ||
| }); | ||
|
|
||
| $model = Model::factory() | ||
| ->create([ | ||
| 'allowed_methods' => 'create', | ||
| ]); | ||
| ->create(); | ||
|
|
There was a problem hiding this comment.
Gate definition mismatch in global perimeter test.
The test is checking for 'view' permission but defines a gate for 'view client models'. For a global perimeter test, the gate should define 'view global models' to properly test the global perimeter logic.
- Gate::define('view client models', function (User $user) {
+ Gate::define('view global models', function (User $user) {
return true;
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Gate::define('view client models', function (User $user) { | |
| return true; | |
| }); | |
| $model = Model::factory() | |
| ->create([ | |
| 'allowed_methods' => 'create', | |
| ]); | |
| ->create(); | |
| Gate::define('view global models', function (User $user) { | |
| return true; | |
| }); | |
| $model = Model::factory() | |
| ->create(); |
Summary by CodeRabbit
New Features
Documentation
Refactor
Bug Fixes
Chores