Harden permission checks and cache behavior#412
Merged
Conversation
Record the implementation plan for the permission package hardening work.\n\nThe plan captures the reviewed decisions, implementation order, testing strategy, and final expected behavior so the PR has a durable reference for the larger change set.
Remove the package-level permission storage connection setting.\n\nThe migrations now use the normal framework migration connection behavior, and the config/test helper no longer expose a separate permission database connection. This keeps permission tables on the same connection path as the app models they reference.
Allow the missing-trait exception to accept any authenticated object.\n\nThe middleware only needs the class name for the error message, so a plain Authenticatable user should receive the package 403 response instead of a TypeError. Add coverage for all three middleware paths.
Compare the opening quote against the end of the full pipe string.\n\nMalformed quoted role strings now fall back to normal pipe splitting instead of being treated as a matched quoted string. Add a regression test for the unmatched quote case.
Make Role reverse-assignment helpers honor the current team context.\n\nThe relation and direct pivot query now include the active team when teams are enabled, so assign, remove, sync, and empty sync do not affect another team's model-role rows. Add focused coverage for model objects, raw ids, and preserving other teams.
Centralize role-assignment cache invalidation through the registrar helpers.\n\nSaved model mutations now rely on the model role cache helper to clear related runtime state, while unsaved queued assignments still clear wildcard state directly because no model cache key exists yet.
Build indexed permission and role lookups from the hydrated catalog.\n\nThe registrar now keeps catalog-order maps, exact key/name indexes, a forbidden role-permission edge flag, and shared runtime cache keys for wildcard and via-role memoization. Model cache forget helpers also clear their related coroutine-local state in one place.
Route read-side role and permission lookups through the registrar catalog indexes.\n\nWrite-side duplicate checks still use the database as the source of truth, and findOrCreate now delegates the create race path to createOrFirst so transaction savepoints and write-PDO refetches come from the framework. Add stale-catalog, race, and team-scoped lookup coverage.
Make permission deny checks guard-exact and resolve concrete permissions before evaluating non-wildcard grants.\n\nRole-deny checks now short-circuit when there are no forbidden role edges, inspect only the requested permission, and use the registrar via-role memo for public materialization paths. Direct and via-role permission getters also dedupe by guard and key while central cache invalidation keeps wildcard and via-role runtime state fresh.
Remove the live docs for the old storage connection option.\n\nDocument that permission getters return effective allowed permissions, explicit denies remain available through deny helpers, undefined cache stores fail fast, and runtime cache scoping covers the via-role memo as well as the other permission cache surfaces.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis PR updates permission matching, catalog lookup, team-scoped reverse assignments, cache invalidation, storage-connection configuration, and supporting tests/docs. ChangesPermission hardening and performance
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant HasPermissions
participant PermissionRegistrar
participant Role
Caller->>HasPermissions: hasPermissionTo(permission)
HasPermissions->>HasPermissions: guardNameForPermissionMatch()
HasPermissions->>HasPermissions: hasForbiddenPermission(permission)
HasPermissions->>HasPermissions: hasForbiddenPermissionViaRoles(permission)
HasPermissions->>PermissionRegistrar: hasForbiddenRolePermissions()
PermissionRegistrar-->>HasPermissions: boolean flag
HasPermissions->>Role: hasPermissionTo(permission)
HasPermissions-->>Caller: allowed or denied
sequenceDiagram
participant Model
participant HasPermissions
participant HasRoles
participant PermissionRegistrar
Model->>HasPermissions: delete()
HasPermissions->>PermissionRegistrar: forgetModelAssignmentCache(model)
Model->>HasRoles: delete()
HasRoles->>PermissionRegistrar: forgetModelAssignmentCache(model)
HasRoles->>HasRoles: assignRole(role)
HasRoles->>PermissionRegistrar: forgetWildcardPermissionIndex(model)
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Greptile SummaryThis PR hardens the permission package's guard handling, team-scoped lookups, and hot-path cache behavior. It fixes guard-exact forbidden-permission checks, adds indexed catalog lookups (
Confidence Score: 5/5The changes are safe to merge — all mutating paths have consistent cache invalidation, and the forbidden-permission guard checks are now exact by guard name throughout. The core logic changes are internally consistent and well-covered by the new tests. Guard-exact forbidden checks prevent cross-guard false denials. The findByParam team filter aligns with the catalog roleMatchesCurrentTeam semantics. Cache invalidation paths are correctly differentiated by whether the catalog itself changed or just a model's assignments changed. No correctness gaps were found. No files require special attention. Important Files Changed
Reviews (3): Last reviewed commit: "fix(permission): restore soft-deleted re..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR hardens the src/permission package’s correctness around guard/team scoping and forbidden-permission semantics, while improving hot-path lookup and cache behavior. It also removes the partially-wired “separate storage connection” configuration surface and updates docs/tests accordingly.
Changes:
- Fix guard-exact forbidden checks, team-scoped reverse assignments, middleware behavior for non-authorizable authenticated users, and pipe-role parsing.
- Add indexed permission/role catalog lookups plus coroutine-local via-role memoization with centralized invalidation.
- Remove
permission.storage.database.connectionfrom config/docs/migrations and add/extend regression tests.
Reviewed changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Permission/Traits/TeamHasRolesTest.php | Adds team-aware role lookup regression tests (global vs current team; catalog-order behavior). |
| tests/Permission/Traits/TeamHasAssignedModelsTest.php | New tests ensuring reverse role assignments (assignToModels/removeFromModels/syncModels) honor current team scoping. |
| tests/Permission/Traits/HasRolesTest.php | Adds regression test for malformed quoted pipe strings. |
| tests/Permission/TestCase.php | Removes obsolete permission.storage.database.connection test config. |
| tests/Permission/Models/FindOrCreateRaceTest.php | New race-condition tests for findOrCreate() returning the existing row after unique constraint conflicts. |
| tests/Permission/Middleware/RoleOrPermissionMiddlewareTest.php | Adds coverage for plain authenticated users lacking authorizable/permission-trait APIs. |
| tests/Permission/Middleware/RoleMiddlewareTest.php | Adds coverage for plain authenticated users lacking authorizable/permission-trait APIs. |
| tests/Permission/Middleware/PermissionMiddlewareTest.php | Adds coverage for plain authenticated users lacking authorizable/permission-trait APIs. |
| tests/Permission/Integration/PermissionRegistrarTest.php | Adds coverage for indexed catalog lookups, stale-catalog write paths, and legacy cache payload compatibility. |
| tests/Permission/ForbiddenPermissionTest.php | Expands guard-exact forbidden behavior tests and de-duping of via-role permissions. |
| tests/Permission/Fixtures/Models/PlainAuthenticatableUser.php | New fixture model implementing only Authenticatable for middleware regression tests. |
| tests/Permission/CacheTest.php | Adds cache invalidation and memoization regression tests (assignment token behavior, via-role memo). |
| src/permission/src/Traits/HasRoles.php | Adjusts delete-hook cache invalidation and avoids redundant wildcard index clearing in saved-model paths; fixes pipe parsing end-quote logic. |
| src/permission/src/Traits/HasPermissions.php | Fixes guard-exact deny semantics (including wildcard flow), narrows role-deny checks, adds via-role memo use, and updates cache invalidation flow. |
| src/permission/src/Traits/HasAssignedModels.php | Makes reverse role assignment relations/pivot queries explicitly team-scoped when teams are enabled. |
| src/permission/src/Support/Config.php | Removes the deprecated storage connection accessor. |
| src/permission/src/PermissionRegistrar.php | Adds catalog indexes (key/name+guard/order), forbidden-edge flag, model via-role memoization, and unified runtime key building. |
| src/permission/src/Models/Role.php | Switches read-side findBy* to indexed registrar lookups and makes findOrCreate resilient via createOrFirst(). |
| src/permission/src/Models/Permission.php | Moves write-side duplicate checks to DB reads and makes findOrCreate resilient via createOrFirst(). |
| src/permission/src/Exceptions/UnauthorizedException.php | Broadens missingTraitHasRoles input type to avoid middleware TypeErrors for plain authenticated users. |
| src/permission/README.md | Updates “Differences” notes to reflect effective-permission getters and cache-store behavior. |
| src/permission/database/migrations/add_teams_fields.php.stub | Removes storage-connection override; uses standard Schema::* calls for migration connection handling. |
| src/permission/database/migrations/2025_07_02_000000_create_permission_tables.php | Removes storage-connection override; uses standard Schema::* calls for migration connection handling. |
| src/permission/config/permission.php | Removes permission.storage.database.connection configuration surface. |
| src/boost/docs/permission.md | Removes “Database Connection” section; updates cache key scoping notes and Differences bullets. |
| docs/plans/2026-07-02-permission-review-hardening-and-performance.md | Adds the detailed hardening/performance plan and acceptance checklist for this work. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/permission/src/Models/Permission.php (1)
55-68: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
create()still has a non-atomic check-then-throw race.The duplicate check now correctly hits the database via
findByParaminstead of the (possibly stale) cache — good fix for the stale-catalog problem. However, unlike the just-hardenedfindOrCreate(),create()still does a plain check-then-insert: two concurrentcreate()calls with the samename/guard_namecan both pass thefindByParamcheck and both proceed toquery()->create($attributes), resulting in either a raw unhandled unique-constraintQueryException(masking the intendedPermissionAlreadyExists) or a duplicate row if no constraint exists.Consider mirroring the
createOrFirst/catch pattern just added tofindOrCreate()here too, catching a unique-constraint violation and re-throwingPermissionAlreadyExistsfor a consistent, race-safe experience across both entry points.♻️ Proposed approach
public static function create(array $attributes = []): PermissionContract { $attributes['guard_name'] ??= Guard::getDefaultName(static::class); $attributes['name'] = enum_value($attributes['name']); $permission = static::findByParam(['name' => $attributes['name'], 'guard_name' => $attributes['guard_name']]); if ($permission) { throw PermissionAlreadyExists::create($attributes['name'], $attributes['guard_name']); } - return static::query()->create($attributes); + try { + return static::query()->create($attributes); + } catch (UniqueConstraintViolationException) { + throw PermissionAlreadyExists::create($attributes['name'], $attributes['guard_name']); + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/permission/src/Models/Permission.php` around lines 55 - 68, The Permission::create() method still does a non-atomic check-then-insert, so concurrent calls can race between findByParam and query()->create($attributes). Update create() to use the same race-safe pattern as findOrCreate()/createOrFirst by catching the unique-constraint failure from static::query()->create and re-throwing PermissionAlreadyExists with the normalized name and guard_name. Keep the duplicate detection in place, but ensure the final insert path handles contention consistently instead of leaking a raw QueryException.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/Permission/Models/FindOrCreateRaceTest.php`:
- Line 13: The FindOrCreateRaceTest class is bypassing transaction rollback by
setting connectionsToTransact to an empty array, so the raced-permission and
raced-role rows remain after the test. Update the test setup to explicitly clean
up those inserted records in the test class lifecycle, using the
FindOrCreateRaceTest and tests/Permission/TestCase patterns so the rows are
deleted before later tests run.
---
Nitpick comments:
In `@src/permission/src/Models/Permission.php`:
- Around line 55-68: The Permission::create() method still does a non-atomic
check-then-insert, so concurrent calls can race between findByParam and
query()->create($attributes). Update create() to use the same race-safe pattern
as findOrCreate()/createOrFirst by catching the unique-constraint failure from
static::query()->create and re-throwing PermissionAlreadyExists with the
normalized name and guard_name. Keep the duplicate detection in place, but
ensure the final insert path handles contention consistently instead of leaking
a raw QueryException.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8f4b65ee-d33c-40ab-87a6-45ec7f616044
📒 Files selected for processing (26)
docs/plans/2026-07-02-permission-review-hardening-and-performance.mdsrc/boost/docs/permission.mdsrc/permission/README.mdsrc/permission/config/permission.phpsrc/permission/database/migrations/2025_07_02_000000_create_permission_tables.phpsrc/permission/database/migrations/add_teams_fields.php.stubsrc/permission/src/Exceptions/UnauthorizedException.phpsrc/permission/src/Models/Permission.phpsrc/permission/src/Models/Role.phpsrc/permission/src/PermissionRegistrar.phpsrc/permission/src/Support/Config.phpsrc/permission/src/Traits/HasAssignedModels.phpsrc/permission/src/Traits/HasPermissions.phpsrc/permission/src/Traits/HasRoles.phptests/Permission/CacheTest.phptests/Permission/Fixtures/Models/PlainAuthenticatableUser.phptests/Permission/ForbiddenPermissionTest.phptests/Permission/Integration/PermissionRegistrarTest.phptests/Permission/Middleware/PermissionMiddlewareTest.phptests/Permission/Middleware/RoleMiddlewareTest.phptests/Permission/Middleware/RoleOrPermissionMiddlewareTest.phptests/Permission/Models/FindOrCreateRaceTest.phptests/Permission/TestCase.phptests/Permission/Traits/HasRolesTest.phptests/Permission/Traits/TeamHasAssignedModelsTest.phptests/Permission/Traits/TeamHasRolesTest.php
💤 Files with no reviewable changes (3)
- src/permission/src/Support/Config.php
- tests/Permission/TestCase.php
- src/permission/config/permission.php
Wrap direct role and permission creation in the database builder's savepoint helper so duplicate insert races are safe inside outer transactions. Keep the existing direct pre-checks so normal duplicate calls still return package exceptions without using database errors as the common path. This is especially important for team roles, where global roles must still block current-team create calls even though the database unique key would not catch that case. Catch unique constraint violations from the final insert and rethrow PermissionAlreadyExists or RoleAlreadyExists. Add forced race tests for both models to prove raw database exceptions do not leak through create().
Add focused coverage for Role::findOrCreate() under teams so the intended effective scope is locked in. The tests prove that a same-name role from another team is not returned and that a current-team role is created instead. They also prove that a global role is considered part of the current effective team scope and is returned without creating a duplicate. This documents the design decision behind global-plus-current role lookup semantics without changing the source behavior.
Add a package-specific database integration test convention under tests/Integration/{Package}/Database/{Driver}. The database workflow now discovers matching package driver directories with find and runs them alongside the framework database integration suite.
Add a Postgres permission integration test for create() duplicate races inside an outer transaction. This is the database behavior the default SQLite suite cannot prove: without withSavepointIfNeeded(), Postgres leaves the transaction aborted after the duplicate insert.
The workflow uses POSIX find rather than Bash-only glob handling, and the path pattern is anchored so it does not duplicate the framework's own tests/Integration/Database/{Driver} directories.
Collaborator
Author
|
Bot review follow-up after f415fbc:
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/permission/src/Models/Role.php (1)
169-189: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
findOrCreate()still rethrows on soft-deletedRolesubclasses.`
createOrFirst()falls back to a plainwhere(...)->first(), so a trashed role matching the same name/guard(/team) is still invisible and the originalUniqueConstraintViolationExceptionbubbles up. For soft-deletableRolemodels, this breaks the “find or create” contract; use awithTrashed()lookup or the restore/create flow here instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/permission/src/Models/Role.php` around lines 169 - 189, findOrCreate() still fails for soft-deletable Role subclasses because the current createOrFirst() fallback ignores trashed records and lets a unique constraint exception bubble up. Update the Role::findOrCreate flow to look up matching roles with trashed records included (or restore the trashed record before returning it) using the existing findByParam/query path and the teams-aware attributes block, so a soft-deleted role with the same name/guard/team is returned instead of rethrowing.
🧹 Nitpick comments (1)
.github/workflows/databases.yml (1)
65-65: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated
find-based phpunit invocation across all 7 jobs.The same discovery pattern is repeated 7 times, differing only by engine string. A composite action or shared shell snippet would reduce copy-paste drift risk as more engines/packages are added, but given the modest scope this is optional.
Also applies to: 121-121, 177-177, 233-233, 289-289, 345-345, 385-385
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/databases.yml at line 65, The phpunit run command is duplicated across multiple database jobs, with only the engine-specific path changing, which makes the workflow harder to maintain. Refactor the repeated vendor/bin/phpunit invocation and its find-based test discovery into a shared reusable shell snippet or composite action, then call that helper from each database job so the MySql and other engine variants only supply their engine-specific argument.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/databases.yml:
- Line 65: The database workflow commands currently tolerate a missing
package-specific test directory, so a typo or rename can silently skip the new
integration suite. Update the repeated phpunit invocations in the databases
workflow to check the result of the find expansion before running tests, and
fail the job loudly if the expected engine-specific directory is missing or
empty. Apply the same guard to each engine job, using the existing phpunit
command blocks as the location to fix.
---
Outside diff comments:
In `@src/permission/src/Models/Role.php`:
- Around line 169-189: findOrCreate() still fails for soft-deletable Role
subclasses because the current createOrFirst() fallback ignores trashed records
and lets a unique constraint exception bubble up. Update the Role::findOrCreate
flow to look up matching roles with trashed records included (or restore the
trashed record before returning it) using the existing findByParam/query path
and the teams-aware attributes block, so a soft-deleted role with the same
name/guard/team is returned instead of rethrowing.
---
Nitpick comments:
In @.github/workflows/databases.yml:
- Line 65: The phpunit run command is duplicated across multiple database jobs,
with only the engine-specific path changing, which makes the workflow harder to
maintain. Refactor the repeated vendor/bin/phpunit invocation and its find-based
test discovery into a shared reusable shell snippet or composite action, then
call that helper from each database job so the MySql and other engine variants
only supply their engine-specific argument.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0837630b-5886-4bc4-9f45-ceb4ccad9ef7
📒 Files selected for processing (6)
.github/workflows/databases.ymlsrc/permission/src/Models/Permission.phpsrc/permission/src/Models/Role.phptests/Integration/Permission/Database/Postgres/PermissionCreateTransactionTest.phptests/Permission/Models/FindOrCreateRaceTest.phptests/Permission/Traits/TeamHasRolesTest.php
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/Permission/Models/FindOrCreateRaceTest.php
- src/permission/src/Models/Permission.php
Handle custom soft-deletable role and permission models in findOrCreate() by using the framework createOrRestore path after the normal active-record lookup misses. This keeps default models on the existing createOrFirst path, keeps create() strict, and preserves normal lookup semantics where trashed records remain hidden from permission checks. Add custom-model tests proving restored records keep their existing role, permission, and model assignment pivots, and document why soft deletes are not recommended for permission models.
Collaborator
Author
|
Follow-up for CodeRabbit review #412 (review) after
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR hardens the permission package and improves its hot-path cache behavior.
It fixes guard-specific deny checks, team-scoped reverse role assignment, middleware errors for plain authenticated users, role pipe parsing, and stale catalog write paths. It also adds indexed catalog lookups, safer
findOrCreate()behavior, local assignment cache invalidation, via-role memoization, and removes the old separate permission storage connection option.For more details, see: docs/plans/2026-07-02-permission-review-hardening-and-performance.md
Tests
composer fixSummary by CodeRabbit