From aa44bb43d5e0ecd76887c6b6d2c23b800416411f Mon Sep 17 00:00:00 2001 From: Raj Siva-Rajah <5361908+binaryfire@users.noreply.github.com> Date: Fri, 3 Jul 2026 03:28:29 +0000 Subject: [PATCH 01/14] docs: add permission hardening plan 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. --- ...ission-review-hardening-and-performance.md | 1508 +++++++++++++++++ 1 file changed, 1508 insertions(+) create mode 100644 docs/plans/2026-07-02-permission-review-hardening-and-performance.md diff --git a/docs/plans/2026-07-02-permission-review-hardening-and-performance.md b/docs/plans/2026-07-02-permission-review-hardening-and-performance.md new file mode 100644 index 000000000..8082941c4 --- /dev/null +++ b/docs/plans/2026-07-02-permission-review-hardening-and-performance.md @@ -0,0 +1,1508 @@ +# Permission Review Hardening And Performance Plan + +## Objective + +Apply the permission package review findings so `src/permission` reads as if it was designed this way from the start: correct guard semantics, team-scoped reverse assignments, hot permission checks that scale with the checked permission rather than the whole catalog, and no half-wired storage mode. + +The final package should have: + +- no cross-guard deny leakage +- no middleware TypeErrors for authenticated users missing the permission trait +- no malformed pipe-string parsing divergence from Spatie +- no cross-team data loss in reverse role assignment helpers +- no global model-assignment cache bust from plain model deletes +- indexed catalog lookups for common role and permission resolutions +- cheap short-circuiting when no role-permission deny edges exist +- narrowed role-deny checks that inspect only the requested permission +- no duplicate rows in `getPermissionsViaRoles()` +- no `permission.storage.database.connection` config surface +- no dead helpers, stale comments, or stale docs left behind + +This is Hypervel 0.4 framework work. Backwards compatibility and diff size are not constraints. The goal is the clean final codebase. + +## Research Summary + +### Source Files Read + +- `src/permission/src/Traits/HasPermissions.php` +- `src/permission/src/Traits/HasRoles.php` +- `src/permission/src/Traits/HasAssignedModels.php` +- `src/permission/src/PermissionRegistrar.php` +- `src/permission/src/Models/Role.php` +- `src/permission/src/Middleware/PermissionMiddleware.php` +- `src/permission/src/Middleware/RoleMiddleware.php` +- `src/permission/src/Middleware/RoleOrPermissionMiddleware.php` +- `src/permission/src/Exceptions/UnauthorizedException.php` +- `src/permission/config/permission.php` +- `src/permission/database/migrations/2025_07_02_000000_create_permission_tables.php` +- `src/permission/database/migrations/add_teams_fields.php.stub` +- `src/permission/src/Support/Config.php` +- `src/database/src/Eloquent/Relations/Concerns/InteractsWithPivotTable.php` +- `src/database/src/Eloquent/Relations/BelongsToMany.php` +- `src/cache/src/StackStore.php` +- `src/boost/docs/cache.md` +- relevant tests under `tests/Permission` +- Spatie reference files under `examples/spatie/permission` + +### Key Findings + +1. `HasPermissions::hasPermissionTo()` checks deny paths before `filterPermission()`. When no guard is passed, `storedPermissionMatches()` skips guard filtering, so a deny under one guard can block an allow under another guard. + +2. A naive default-guard fix would create a fail-open bug for concrete `Permission` objects. If an API-guard `Permission` object is passed while the model default is web, forcing the default guard before matching would hide API forbidden rows. + +3. `PermissionMiddleware`, `RoleOrPermissionMiddleware`, and `RoleMiddleware` can call `UnauthorizedException::missingTraitHasRoles(Authorizable $user)` with an authenticated object that is not `Authorizable`. The exception only needs `$user::class`. + +4. `HasRoles::convertPipeToArray()` has a porting typo: it compares the first character to itself instead of the last character of the pipe string. + +5. `HasAssignedModels::syncModels()` deletes every `model_has_roles` row for the role, across all teams. `assignToModels()` and `removeFromModels()` also need current-team scoping when teams are enabled. + +6. `bootHasRoles()` and `bootHasPermissions()` bump the global model-assignment cache token for every delete. Plain model deletes only need local assignment-cache clearing. + +7. `PermissionRegistrar::getPermissions()` and `getRoles()` scan the whole hydrated catalog for common exact lookups. This affects `findByName()`, `findById()`, `filterPermission()`, and direct assignment hydration. + +8. `hasForbiddenPermissionViaRoles()` materializes every permission's role pivots on every role-deny check. + +9. Most deployments have no role-permission deny edges. A catalog-level boolean can skip the role-deny path cheaply. + +10. `getPermissionsViaRoles()` can return duplicate permissions when multiple held roles grant the same permission. `getAllPermissions()` already dedupes. + +11. `permission.storage.database.connection` is broken and should be removed, not completed. Migrations use it, models do not, and the default defeats normal `php artisan migrate --database=...` behavior. + +12. Hypervel's `stack` cache driver is the right hot-read tool for permission catalogs. A separate permission DB connection adds a second pool and breaks transaction coherence with app models. + +## Agreed Decisions + +1. Fix guard matching by resolving the effective guard per input type: + + - explicit guard argument wins + - concrete `Permission` object uses its own `guard_name` + - all other inputs use the model default guard + +2. Keep deny checks fail-closed without making concrete permission objects fail open. + +3. For non-wildcard `hasPermissionTo()`, resolve the concrete permission once with `filterPermission()` before direct and role grant checks. Public deny helper methods still resolve their own effective guard because callers can use them directly. + +4. Widen `UnauthorizedException::missingTraitHasRoles()` to `object`. + +5. Treat `HasAssignedModels` team scoping as a correctness/data-loss fix. + +6. Remove the global assignment-token bump from delete hooks. Plain model deletes clear only the deleted model's assignment cache. + +7. Add registrar catalog indexes instead of a separate direct-permission hydrated memo. This fixes the root O(catalog) lookup cost. + +8. Add a serialized `hasForbiddenRolePermissions` catalog flag. + +9. Narrow `hasForbiddenPermissionViaRoles()` to the requested permission and effective guard. + +10. Add a coroutine-local via-role materialization memo for public getters only, with centralized invalidation. + +11. Remove `permission.storage.database.connection` completely. + +12. Remove stale docs/comments/code. Do not leave compatibility shims for removed behavior. + +## Implementation Order + +### 1. Cross-Guard Deny Semantics + +#### Why + +`storedPermissionMatches()` only applies the guard filter when `$guardName !== null`: + +```php +protected function storedPermissionMatches(Model $storedPermission, mixed $permission, ?string $guardName = null): bool +{ + if ($guardName !== null && $storedPermission->getAttribute('guard_name') !== $guardName) { + return false; + } + + if ($permission instanceof Permission) { + return $storedPermission->getKey() === $permission->getKey(); + } + + // ... +} +``` + +`hasPermissionTo('edit')` passes `null` to both forbidden checks before the allow path resolves the default guard. That means `edit@api` denied can block `edit@web` allowed in default-web contexts. + +#### How + +Add an effective guard helper near `storedPermissionMatches()`: + +```php +/** + * Resolve the guard to use when matching stored permissions. + */ +protected function guardNameForPermissionMatch(mixed $permission, ?string $guardName = null): string +{ + if ($guardName !== null) { + return $guardName; + } + + if ($permission instanceof Permission) { + $permissionGuard = $permission->guard_name ?? null; + + if (is_string($permissionGuard) && $permissionGuard !== '') { + return $permissionGuard; + } + } + + return $this->getDefaultGuardName(); +} +``` + +Update public forbidden helper methods to use it: + +```php +public function hasForbiddenPermission($permission, ?string $guardName = null): bool +{ + $guardName = $this->guardNameForPermissionMatch($permission, $guardName); + + return $this->getCachedDirectPermissions() + ->contains( + fn (Model $storedPermission): bool => $this->pivotIsForbidden($storedPermission) + && $this->storedPermissionMatches($storedPermission, $permission, $guardName) + ); +} +``` + +```php +public function hasForbiddenPermissionViaRoles($permission, ?string $guardName = null): bool +{ + if ($this instanceof Role || $this instanceof Permission) { + return false; + } + + $guardName = $this->guardNameForPermissionMatch($permission, $guardName); + + // This method is further rewritten in step 7. +} +``` + +Update non-wildcard `hasPermissionTo()` to resolve once: + +```php +public function hasPermissionTo($permission, ?string $guardName = null): bool +{ + if ($this->getWildcardClass()) { + if ($this->hasForbiddenPermission($permission, $guardName)) { + return false; + } + + if ($this->hasForbiddenPermissionViaRoles($permission, $guardName)) { + return false; + } + + return $this->hasWildcardPermission($permission, $guardName); + } + + $permission = $this->filterPermission($permission, $guardName); + + if ($this->hasForbiddenPermission($permission, $guardName)) { + return false; + } + + if ($this->hasForbiddenPermissionViaRoles($permission, $guardName)) { + return false; + } + + return $this->hasDirectPermission($permission) || $this->hasPermissionViaRole($permission); +} +``` + +Apply the same resolve-once shape to `Role::hasPermissionTo()`: + +```php +public function hasPermissionTo(UnitEnum|int|string|PermissionContract $permission, ?string $guardName = null): bool +{ + if ($this->getWildcardClass()) { + if ($this->hasForbiddenPermission($permission, $guardName)) { + return false; + } + + return $this->hasWildcardPermission($permission, $guardName); + } + + $permission = $this->filterPermission($permission, $guardName); + + if ($this->hasForbiddenPermission($permission, $guardName)) { + return false; + } + + if (! $this->getGuardNames()->contains($permission->guard_name)) { + throw GuardDoesNotMatch::create($permission->guard_name, $guardName ? collect([$guardName]) : $this->getGuardNames()); + } + + // Existing relation check remains, but now runs after guard-exact deny. +} +``` + +Keep `storedPermissionMatches()` as the final comparison helper. It can still accept nullable guard, but every deny path should pass an effective guard. + +#### Tests + +Add tests to `tests/Permission/ForbiddenPermissionTest.php`: + +- same permission name exists under `web` and `api` +- user has allow for `web` +- user has direct deny for `api` +- default guard is `web` +- `hasPermissionTo('permission-name')` returns true +- `hasPermissionTo('permission-name', 'api')` returns false +- passing the concrete API permission object returns false +- passing the concrete web permission object returns true + +Add role-path equivalents: + +- user has a web role allowing `edit` +- user has an api role denying `edit` +- default web string check is true +- explicit api string check is false +- concrete API permission object is false + +Add `Role::hasPermissionTo()` tests: + +- role with API forbidden permission checked by concrete API permission object returns false +- role with default web context does not miss the concrete object's deny + +Run immediately: + +```shell +./vendor/bin/phpunit --no-progress tests/Permission/ForbiddenPermissionTest.php +``` + +### 2. Middleware TypeError + +#### Why + +Middleware can call: + +```php +throw UnauthorizedException::missingTraitHasRoles($user); +``` + +with an authenticated object that is not `Authorizable`. The exception method only reads the class name: + +```php +public static function missingTraitHasRoles(Authorizable $user): static +{ + return new static(403, __('Authorizable class `:class` must use Hypervel\Permission\Traits\HasRoles trait.', [ + 'class' => $user::class, + ]), null, []); +} +``` + +That should be a 403 package exception, not a TypeError. + +#### How + +Change the exception signature: + +```php +/** + * Create an exception for a user missing the HasRoles trait. + */ +public static function missingTraitHasRoles(object $user): static +{ + return new static(403, __('Authorizable class `:class` must use Hypervel\Permission\Traits\HasRoles trait.', [ + 'class' => $user::class, + ]), null, []); +} +``` + +No middleware logic change is needed. + +#### Tests + +Add tests in: + +- `tests/Permission/Middleware/PermissionMiddlewareTest.php` +- `tests/Permission/Middleware/RoleMiddlewareTest.php` +- `tests/Permission/Middleware/RoleOrPermissionMiddlewareTest.php` + +Each test should authenticate a model/object that implements `Authenticatable` enough for the guard but does not implement `Authorizable` or the relevant permission trait methods. Assert `UnauthorizedException` is thrown, not `TypeError`. + +Run after each file: + +```shell +./vendor/bin/phpunit --no-progress tests/Permission/Middleware/PermissionMiddlewareTest.php +./vendor/bin/phpunit --no-progress tests/Permission/Middleware/RoleMiddlewareTest.php +./vendor/bin/phpunit --no-progress tests/Permission/Middleware/RoleOrPermissionMiddlewareTest.php +``` + +### 3. Pipe String Parsing Typo + +#### Why + +Hypervel currently has: + +```php +$quoteCharacter = substr($pipeString, 0, 1); +$endCharacter = substr($quoteCharacter, -1, 1); +``` + +Spatie uses: + +```php +$endCharacter = substr($pipeString, -1, 1); +``` + +The current Hypervel code makes `$quoteCharacter !== $endCharacter` dead. + +#### How + +Change only the second line: + +```php +$endCharacter = substr($pipeString, -1, 1); +``` + +#### Tests + +Add or update a role test covering malformed quoted role strings: + +```php +$this->assertFalse($user->hasRole('"admin|editor')); +``` + +The malformed quoted string should be treated as a normal pipe-delimited string, not trimmed as if it had matching quotes. + +Run: + +```shell +./vendor/bin/phpunit --no-progress tests/Permission/Traits/HasRolesTest.php +``` + +If the test file has a different name, use the existing file that covers `hasRole()` and pipe strings. + +### 4. Team-Scope `HasAssignedModels` + +#### Why + +`HasAssignedModels` is a reverse assignment API on `Role`. It must honor the current team when teams are enabled. + +Current bugs: + +```php +$existingIds = $relation->pluck(Config::morphKey())->all(); +$relation->attach(array_diff($ids, $existingIds), $teamPivot); +``` + +`assignToModels()` can skip attaching a current-team row because another team already has the same model id. + +```php +$this->relationForModel($morphClass)->detach($ids); +``` + +`removeFromModels()` depends on relation state for team scoping and should be explicit. + +```php +$this->newPivotQueryForRole()->delete(); +``` + +`syncModels()` deletes every model assignment for the role across every team. + +#### Required Source Check Before Coding + +Before implementing this step, re-read: + +- `src/permission/src/Traits/HasAssignedModels.php` +- `src/permission/src/Traits/HasRoles.php::roles()` +- `src/database/src/Eloquent/Relations/Concerns/InteractsWithPivotTable.php::detach()` +- `src/database/src/Eloquent/Relations/Concerns/InteractsWithPivotTable.php::newPivotQuery()` +- `src/database/src/Eloquent/Relations/BelongsToMany.php::wherePivot()` + +`newPivotQuery()` applies `pivotWheres`, so a relation returned from `relationForModel()` with `wherePivot($teamsKey, getPermissionsTeamId())` will scope `pluck()` and `detach()`. + +#### How + +Make `relationForModel()` team-aware: + +```php +protected function relationForModel(string $modelClass): MorphToMany +{ + $relation = $this->morphedByMany( + $modelClass, + 'model', + Config::modelHasRolesTable(), + Container::getInstance()->make(PermissionRegistrar::class)->pivotRole, + Config::morphKey(), + ); + + if (! Config::teamsEnabled()) { + return $relation; + } + + return $relation->wherePivot(Config::teamForeignKey(), getPermissionsTeamId()); +} +``` + +Make `newPivotQueryForRole()` team-aware: + +```php +private function newPivotQueryForRole(): Builder +{ + $query = $this->getConnection() + ->table(Config::modelHasRolesTable()) + ->where(Container::getInstance()->make(PermissionRegistrar::class)->pivotRole, $this->getKey()); + + if (Config::teamsEnabled()) { + $query->where(Config::teamForeignKey(), getPermissionsTeamId()); + } + + return $query; +} +``` + +Keep `teamPivot()` as the single source for attach pivot values: + +```php +private function teamPivot(): array +{ + if (! Config::teamsEnabled()) { + return []; + } + + return [Config::teamForeignKey() => getPermissionsTeamId()]; +} +``` + +Do not remove the global bumps in `assignToModels()`, `removeFromModels()`, or `syncModels()`. These methods mutate assignments for arbitrary models, so local invalidation is not enough. + +#### Tests + +Create a teams-specific reverse assignment test file if no suitable one exists: + +- `tests/Permission/Traits/TeamHasAssignedModelsTest.php` + +This file should enable teams like `TeamHasPermissionsTest`. + +Required tests: + +1. `assignToModels()` attaches in the current team even when the same user already has the role in another team. +2. `removeFromModels()` removes only the current team assignment. +3. `syncModels()` replaces only current team assignments and leaves other team rows intact. +4. Empty `syncModels([])` clears only current team rows. +5. Raw id paths with explicit model class also honor current team. + +Run immediately: + +```shell +./vendor/bin/phpunit --no-progress tests/Permission/Traits/TeamHasAssignedModelsTest.php +``` + +Then run the existing reverse assignment tests: + +```shell +./vendor/bin/phpunit --no-progress tests/Permission/Traits/HasAssignedModelsTest.php +``` + +### 5. Delete-Hook Assignment Cache Invalidation + +#### Why + +Plain model deletes currently rotate the global model-assignment cache token: + +```php +$registrar->bumpModelAssignmentCacheToken(); +``` + +That invalidates every model's role and permission assignment caches when any user is deleted. + +Role and Permission model deletes already trigger: + +```php +RefreshesPermissionCache::deleted() -> forgetCachedPermissions() +``` + +which clears the catalog and bumps the assignment token. + +#### How + +In `HasRoles::bootHasRoles()`: + +```php +if ($model instanceof Permission) { + // existing shared row cleanup +} else { + // existing model_has_roles cleanup + $registrar->forgetModelAssignmentCache($model); +} +``` + +Remove the unconditional token bump from that delete hook. + +In `HasPermissions::bootHasPermissions()`: + +```php +if (! $model instanceof Permission && ! $model instanceof Role) { + // existing model_has_permissions cleanup + Container::getInstance()->make(PermissionRegistrar::class)->forgetModelAssignmentCache($model); +} +``` + +Remove the unconditional token bump from that delete hook. + +Do not add extra cache clearing for Role or Permission deletes in these trait hooks. `RefreshesPermissionCache` owns the global catalog/token invalidation. + +#### Tests + +Add tests in `tests/Permission/CacheTest.php`: + +1. Warm assignment caches for two users. +2. Capture `PermissionRegistrar::modelAssignmentCacheToken()`. +3. Delete one user. +4. Assert the token did not change. +5. Assert the other user's permission/role check still reads correctly. + +Add a second test: + +1. Capture token. +2. Delete a Role or Permission. +3. Assert token changes through `RefreshesPermissionCache`. + +Run: + +```shell +./vendor/bin/phpunit --no-progress tests/Permission/CacheTest.php +``` + +### 6. Registrar Catalog Indexes + +#### Why + +Common lookups currently do O(catalog) scans: + +```php +return $this->filterModels($permissions, $params, $onlyOne); +``` + +That affects: + +- `Permission::findByName()` +- `Permission::findById()` +- `Role::findByName()` +- `Role::findById()` +- `filterPermission()` +- `getCachedDirectPermissions()` + +The catalog is already hydrated once per coroutine. Indexes should be built in that same lifecycle. + +#### How + +Change the catalog shape from: + +```php +/** + * @return array{permissions: Collection, roles: Collection} + */ +private function permissionCatalog(): array +``` + +to: + +```php +/** + * @return array{ + * permissions: Collection, + * roles: Collection, + * permissionByKey: array, + * permissionByNameAndGuard: array>, + * permissionOrderByKey: array, + * roleByKey: array, + * roleByNameAndGuard: array>, + * roleOrderByKey: array, + * hasForbiddenRolePermissions: bool + * } + */ +private function permissionCatalog(): array +``` + +Build indexes after hydration: + +```php +$roles = $this->getHydratedRoleCollection($payload['roles']); +$permissions = $this->getHydratedPermissionCollection($payload['permissions'], $roles); + +$catalog = [ + 'roles' => $roles, + 'permissions' => $permissions, + 'permissionByKey' => $this->indexModelsByKey($permissions), + 'permissionByNameAndGuard' => $this->indexModelsByNameAndGuard($permissions), + 'permissionOrderByKey' => $this->indexModelOrderByKey($permissions), + 'roleByKey' => $this->indexModelsByKey($roles), + 'roleByNameAndGuard' => $this->indexModelsByNameAndGuard($roles), + 'roleOrderByKey' => $this->indexModelOrderByKey($roles), + 'hasForbiddenRolePermissions' => (bool) ($payload['hasForbiddenRolePermissions'] ?? false), +]; +``` + +Add helpers: + +```php +/** + * @return array + */ +private function indexModelsByKey(Collection $models): array +{ + return $models + ->mapWithKeys(fn (Model $model): array => [(string) $model->getKey() => $model]) + ->all(); +} +``` + +```php +/** + * @return array> + */ +private function indexModelsByNameAndGuard(Collection $models): array +{ + $indexed = []; + + foreach ($models as $model) { + $indexed[$this->nameGuardIndexKey($model->getAttribute('name'), $model->getAttribute('guard_name'))][] = $model; + } + + return $indexed; +} +``` + +```php +private function nameGuardIndexKey(mixed $name, mixed $guardName): string +{ + return (string) $guardName . "\0" . (string) $name; +} +``` + +Use null-byte as an internal separator so names containing `|` do not collide. + +Add a stable catalog-order map so list lookups can use indexes without changing public collection order: + +```php +/** + * @return array + */ +private function indexModelOrderByKey(Collection $models): array +{ + $order = []; + + foreach ($models->values() as $index => $model) { + $order[(string) $model->getKey()] = $index; + } + + return $order; +} +``` + +Add fast-path filtering: + +```php +protected function indexedModels(array $params, bool $onlyOne, string $modelType): ?Collection +{ + if ($params === []) { + return null; + } + + $catalog = $this->permissionCatalog(); + $keyName = $modelType === 'permission' + ? Guard::getModelKeyName($this->permissionClass) + : Guard::getModelKeyName($this->roleClass); + + $byKey = $catalog[$modelType === 'permission' ? 'permissionByKey' : 'roleByKey']; + $byNameAndGuard = $catalog[$modelType === 'permission' ? 'permissionByNameAndGuard' : 'roleByNameAndGuard']; + $orderByKey = $catalog[$modelType === 'permission' ? 'permissionOrderByKey' : 'roleOrderByKey']; + + if (array_key_exists($keyName, $params) && count(array_diff_key($params, [$keyName => true, 'guard_name' => true])) === 0) { + $ids = Arr::wrap($params[$keyName]); + $guardNames = array_key_exists('guard_name', $params) ? Arr::wrap($params['guard_name']) : null; + $matches = []; + + foreach ($ids as $id) { + $model = $byKey[(string) $id] ?? null; + + if (! $model instanceof Model) { + continue; + } + + if ($guardNames !== null && ! self::attributeMatches($model->getAttribute('guard_name'), $guardNames)) { + continue; + } + + if ($guardNames !== null && $modelType === 'role' && ! $this->roleMatchesCurrentTeam($model)) { + continue; + } + + $matches[(string) $model->getKey()] = $model; + } + + return $this->collectionFromIndexedModels($matches, $orderByKey, $onlyOne); + } + + if (array_key_exists('name', $params) && array_key_exists('guard_name', $params) && count($params) === 2) { + $names = Arr::wrap($params['name']); + $guards = Arr::wrap($params['guard_name']); + $matches = []; + + foreach ($guards as $guardName) { + foreach ($names as $name) { + foreach ($byNameAndGuard[$this->nameGuardIndexKey($name, $guardName)] ?? [] as $model) { + if ($modelType === 'role' && ! $this->roleMatchesCurrentTeam($model)) { + continue; + } + + $matches[(string) $model->getKey()] = $model; + } + } + } + + return $this->collectionFromIndexedModels($matches, $orderByKey, $onlyOne); + } + + return null; +} +``` + +Add the role team filter used by indexed `Role::findByName()` and `Role::findById()` lookups: + +```php +private function roleMatchesCurrentTeam(Model $role): bool +{ + if (! $this->teams) { + return true; + } + + $teamId = $this->getPermissionsTeamId(); + $roleTeamId = $role->getAttribute($this->teamsKey); + + return $roleTeamId === null || self::attributeMatches($roleTeamId, $teamId); +} +``` + +This mirrors `Role::findByParam()`'s existing team semantics: when teams are enabled, role lookup accepts global roles (`team_id` null) and roles matching the current team. The final one-item result still comes from catalog order, which matches the existing query's unordered first-row behavior without adding a new priority rule. + +Add the collection builder used by both indexed branches: + +```php +/** + * @param array $models + * @param array $orderByKey + */ +private function collectionFromIndexedModels(array $models, array $orderByKey, bool $onlyOne): Collection +{ + uasort($models, fn (Model $a, Model $b): int => ($orderByKey[(string) $a->getKey()] ?? PHP_INT_MAX) + <=> ($orderByKey[(string) $b->getKey()] ?? PHP_INT_MAX)); + + $collection = new Collection(array_values($models)); + + return $onlyOne ? $collection->take(1)->values() : $collection->values(); +} +``` + +Keep the public methods behavior-preserving: + +```php +public function getPermissions(array $params = [], bool $onlyOne = false, ?string $permissionClass = null): Collection +{ + if ($permissionClass !== null && $permissionClass !== $this->permissionClass) { + return $this->filterModels($this->getPermissionsWithRoles($permissionClass), $params, $onlyOne); + } + + $indexed = $this->indexedModels($params, $onlyOne, 'permission'); + + if ($indexed !== null) { + return $indexed; + } + + return $this->filterModels($this->permissionCatalog()['permissions'], $params, $onlyOne); +} +``` + +Do the same for `getRoles()`. + +Update `Role::findByName()` and `Role::findById()` to use the registrar catalog instead of a direct DB query: + +```php +protected static function getRoles(array $params = [], bool $onlyOne = false): Collection +{ + return Container::getInstance()->make(PermissionRegistrar::class) + ->getRoles($params, $onlyOne, static::class); +} +``` + +```php +protected static function getRole(array $params = []): ?RoleContract +{ + /** @var null|RoleContract */ + return static::getRoles($params, true)->first(); +} +``` + +Then use `static::getRole(['name' => $name, 'guard_name' => $guardName])` and `static::getRole([Guard::getModelKeyName(static::class) => $id, 'guard_name' => $guardName])` in `findByName()` and `findById()`. Keep `findByParam()` for `findOrCreate()` and `create()` because those are write-side uniqueness checks where the direct DB query remains the authoritative guard before insert. + +Apply the same write-side rule to `Permission::create()`. It currently checks uniqueness through `getPermission()`, which reads the coroutine-local catalog. If another coroutine creates the same permission after this coroutine hydrated its catalog, the pre-check can miss and the insert surfaces a raw database unique-constraint exception instead of `PermissionAlreadyExists`. + +Add a direct DB lookup helper for permission creation: + +```php +protected static function findByParam(array $params = []): ?PermissionContract +{ + $query = static::query(); + + foreach ($params as $key => $value) { + $query->where($key, $value); + } + + return $query->first(); +} +``` + +Then update `Permission::create()` to use `static::findByParam(['name' => $attributes['name'], 'guard_name' => $attributes['guard_name']])` for its duplicate check. Update `Permission::findOrCreate()` to use the same direct DB lookup before creating, so a stale coroutine catalog returns the existing permission instead of delegating to `create()` and receiving `PermissionAlreadyExists`. Keep `getPermission()` for `findByName()` and `findById()` because those are read-side APIs that should benefit from the catalog. + +Catalog-resolved roles now behave like catalog-resolved permissions: they are hydrated from the cache payload, so attributes listed in `permission.cache.column_names_except` are absent. By default that means `created_at`, `updated_at`, and `deleted_at` are not present on roles returned by `Role::findByName()` and `Role::findById()`. This is intentional for the read-side catalog path; tests should verify behavior through key/name/guard/team data rather than reintroducing timestamps into the catalog. + +Preserve return type and visible behavior: + +- return `Collection` +- `onlyOne` returns empty collection or a one-item collection +- list lookups should preserve catalog order unless a test proves callers do not observe the order +- fall back to `filterModels()` for any param shape not exactly handled +- use `!== null` for the indexed fast path so an empty lookup result is still returned directly + +#### Add Forbidden Edge Flag + +Update serialized payload: + +```php +private function getSerializedPermissionsForCache(): array +{ + $except = $this->config->array('permission.cache.column_names_except', ['created_at', 'updated_at', 'deleted_at']); + $permissions = $this->getPermissionsWithRoles(); + $hasForbiddenRolePermissions = false; + + return [ + 'permissions' => $permissions + ->map(function (Model $permission) use (&$hasForbiddenRolePermissions, $except): array { + $roles = $this->relationCollection($permission, 'roles') + ->map(function (Model $role) use ($permission, &$hasForbiddenRolePermissions): array { + $isForbidden = $this->pivotIsForbidden($role); + $hasForbiddenRolePermissions = $hasForbiddenRolePermissions || $isForbidden; + + return [ + 'pivot' => [ + $this->pivotPermission => $permission->getKey(), + $this->pivotRole => $role->getKey(), + 'is_forbidden' => $isForbidden, + ], + ]; + }) + ->values() + ->all(); + + return [ + 'attributes' => Arr::except($permission->getAttributes(), $except), + 'roles' => $roles, + ]; + }) + ->values() + ->all(), + 'roles' => $this->getRolesForCache() + ->map(fn (Model $role): array => [ + 'attributes' => Arr::except($role->getAttributes(), $except), + ]) + ->values() + ->all(), + 'hasForbiddenRolePermissions' => $hasForbiddenRolePermissions, + ]; +} +``` + +Because this changes only the value stored under the package cache key and the package already tolerates recaching, no migration is needed. The hydration path should use `?? false` so old cache payloads are harmless until they expire or are flushed. + +#### Tests + +Add registrar tests in the existing `tests/Permission/Integration/PermissionRegistrarTest.php` when they target registrar lookup behavior. Use `tests/Permission/CacheTest.php` only for cache invalidation behavior. + +1. `findByName()` returns the correct permission for same name across guards. +2. `findById()` with guard returns the correct permission. +3. list lookup by ids preserves expected order. +4. missing lookup returns an empty collection through registrar and still throws through model `findBy*`. +5. role lookups by name and key use the registrar catalog and still throw the same exceptions when missing. +6. team-enabled role lookup still finds global roles and current-team roles, and does not find another team's role. +7. when a global role and current-team role share name and guard, lookup preserves the existing first-row/catalog-order behavior rather than inventing a new priority rule. +8. `Role::findOrCreate()` and `Role::create()` still use the write-side DB uniqueness path. +9. stale coroutine catalog plus a duplicate `Permission::create()` still throws `PermissionAlreadyExists`, not a raw database unique-constraint exception. +10. stale coroutine catalog plus `Permission::findOrCreate()` returns the existing DB row instead of throwing. +11. catalog-resolved roles expose the expected key/name/guard/team data and do not require timestamp attributes that the catalog intentionally strips. +12. old cache payload without `hasForbiddenRolePermissions` is treated as false if practical to seed. + +Performance behavior can be tested by creating unrelated permissions and asserting a permission check result remains correct. Avoid brittle tests that depend on private loop counts unless a clean spy pattern already exists. + +Run: + +```shell +./vendor/bin/phpunit --no-progress tests/Permission/CacheTest.php +``` + +Run the registrar test immediately after updating it: + +```shell +./vendor/bin/phpunit --no-progress tests/Permission/Integration/PermissionRegistrarTest.php +``` + +### 7. Narrow Role-Deny Checks + +#### Why + +Current role-deny check: + +```php +return $this->getPermissionsViaRolesWithPivots() + ->contains( + fn (Model $storedPermission): bool => $this->pivotIsForbidden($storedPermission) + && $this->storedPermissionMatches($storedPermission, $permission, $guardName) + ); +``` + +`getPermissionsViaRolesWithPivots()` walks the whole permission catalog. A check for one permission should inspect one permission. + +#### How + +Add registrar method: + +```php +/** + * Determine if any cached role-permission edge is forbidden. + */ +public function hasForbiddenRolePermissions(): bool +{ + return (bool) $this->permissionCatalog()['hasForbiddenRolePermissions']; +} +``` + +Add a no-throw permission resolver in `HasPermissions`: + +```php +protected function permissionForMatch(mixed $permission, string $guardName): ?Model +{ + $permissionKey = Guard::getModelKeyName($this->getPermissionClass()); + + if ($permission instanceof Permission) { + if (! $permission instanceof Model || $permission->guard_name !== $guardName) { + return null; + } + + return $this->permissionRegistrar() + ->getPermissions([$permissionKey => $permission->getKey(), 'guard_name' => $guardName], true, $this->getPermissionClass()) + ->first(); + } + + $permission = enum_value($permission); + + if (! is_string($permission) && ! is_int($permission)) { + return null; + } + + $params = is_int($permission) || PermissionRegistrar::isUid($permission) + ? [$permissionKey => $permission, 'guard_name' => $guardName] + : ['name' => $permission, 'guard_name' => $guardName]; + + return $this->permissionRegistrar() + ->getPermissions($params, true, $this->getPermissionClass()) + ->first(); +} +``` + +Then rewrite `hasForbiddenPermissionViaRoles()`: + +```php +public function hasForbiddenPermissionViaRoles($permission, ?string $guardName = null): bool +{ + if ($this instanceof Role || $this instanceof Permission) { + return false; + } + + $roles = $this->getCachedRoles(); + + if ($roles->isEmpty()) { + return false; + } + + $registrar = $this->permissionRegistrar(); + + if (! $registrar->hasForbiddenRolePermissions()) { + return false; + } + + $guardName = $this->guardNameForPermissionMatch($permission, $guardName); + $storedPermission = $this->permissionForMatch($permission, $guardName); + + if (! $storedPermission instanceof Model) { + return false; + } + + $roleIds = array_flip($roles->map(fn (Model $role): string => (string) $role->getKey())->all()); + + return $this->relationCollection($storedPermission, 'roles') + ->contains( + fn (Model $role): bool => isset($roleIds[(string) $role->getKey()]) + && $this->pivotIsForbidden($role) + ); +} +``` + +Keep the fast ordering: + +1. role/permission model early return +2. no cached roles early return +3. no forbidden role edges early return +4. resolve requested permission +5. inspect only that permission's role pivots + +#### Tests + +Add to `tests/Permission/ForbiddenPermissionTest.php`: + +- forbidden role permission under another guard does not deny default guard +- forbidden role permission for a different permission does not deny requested permission +- explicit guard with role deny still denies +- concrete permission object from denied guard is denied +- missing permission still returns false from `checkPermissionTo()` and throws from `hasPermissionTo()` as before +- no roles returns false without needing catalog side effects + +Run: + +```shell +./vendor/bin/phpunit --no-progress tests/Permission/ForbiddenPermissionTest.php +``` + +### 8. Via-Role Materialization Memo And Deduping + +#### Why + +`getPermissionsViaRoles()` and `getAllPermissions()` call `getPermissionsViaRolesWithPivots()`, which materializes the full permission set through held roles. After step 7, that materializer is no longer used by the hot `hasPermissionTo()` deny path, but public getters can call it repeatedly in one coroutine. + +`getPermissionsViaRoles()` also lacks `unique()`. + +#### How + +Add a new context key: + +```php +public const MODEL_VIA_ROLE_PERMISSIONS_CONTEXT_KEY = '__permission.model_via_role_permissions'; +``` + +Add registrar helpers: + +```php +public function rememberModelViaRolePermissions(Model $model, Closure $callback): Collection +{ + $key = $this->modelRuntimeCacheKey($model); + $items = CoroutineContext::get(self::MODEL_VIA_ROLE_PERMISSIONS_CONTEXT_KEY, []); + + if (isset($items[$key]) && $items[$key] instanceof Collection) { + return $items[$key]; + } + + $items[$key] = $callback(); + CoroutineContext::set(self::MODEL_VIA_ROLE_PERMISSIONS_CONTEXT_KEY, $items); + + return $items[$key]; +} +``` + +```php +public function forgetModelViaRolePermissions(Model $model): void +{ + $items = CoroutineContext::get(self::MODEL_VIA_ROLE_PERMISSIONS_CONTEXT_KEY, []); + unset($items[$this->modelRuntimeCacheKey($model)]); + CoroutineContext::set(self::MODEL_VIA_ROLE_PERMISSIONS_CONTEXT_KEY, $items); +} +``` + +```php +protected function modelRuntimeCacheKey(Model $model): string +{ + $teamId = $this->teams ? (string) ($this->getPermissionsTeamId() ?? 'global') : 'none'; + $segments = [ + $this->modelAssignmentCacheToken(), + $model->getMorphClass(), + $model->getKey(), + $teamId, + ]; + + $scope = $this->cacheKeyScopeSegment(); + + if ($scope !== '') { + array_unshift($segments, $scope); + } + + return implode(':', $segments); +} +``` + +Replace the body of `wildcardPermissionIndexKey()` with this helper too: + +```php +protected function wildcardPermissionIndexKey(Model $record): string +{ + return $this->modelRuntimeCacheKey($record); +} +``` + +This avoids two independent copies of the same runtime key formula drifting apart. + +Clear this context in `clearPermissionsCollection()`: + +```php +CoroutineContext::forget(self::MODEL_VIA_ROLE_PERMISSIONS_CONTEXT_KEY); +``` + +Extend `forgetModelAssignmentCache()`: + +```php +public function forgetModelAssignmentCache(Model $model): void +{ + $cache = $this->cacheRepository(); + + $cache->forget($this->modelCacheKey($this->modelRolesCacheKeyPrefix, $model)); + $cache->forget($this->modelCacheKey($this->modelPermissionsCacheKeyPrefix, $model)); + + $this->forgetModelViaRolePermissions($model); + $this->forgetWildcardPermissionIndex($model); +} +``` + +Extend `forgetModelRoleCache()`: + +```php +public function forgetModelRoleCache(Model $model): void +{ + $this->cacheRepository()->forget( + $this->modelCacheKey($this->modelRolesCacheKeyPrefix, $model) + ); + + $this->forgetModelViaRolePermissions($model); + $this->forgetWildcardPermissionIndex($model); +} +``` + +Extend `forgetModelPermissionCache()` so direct permission mutations also clear the wildcard index in one place: + +```php +public function forgetModelPermissionCache(Model $model): void +{ + $this->cacheRepository()->forget( + $this->modelCacheKey($this->modelPermissionsCacheKeyPrefix, $model) + ); + + $this->forgetWildcardPermissionIndex($model); +} +``` + +After centralizing wildcard invalidation in the registrar, remove redundant `$this->forgetWildcardPermissionIndex()` calls from trait mutation methods that immediately called one of these registrar forget helpers. Keep direct calls only where no registrar model-cache helper is invoked. + +Apply this exact keep/remove list: + +| File / method | Current site | Action | Why | +|---|---:|---|---| +| `HasPermissions::attachPermissions()` | around line 644 | Keep for unsaved models; remove from branches that already called `forgetModelPermissionCache()` or `forgetCachedPermissions()` by restructuring the method so the direct call only runs when no cache-clearing helper ran | unsaved models only queue assignments and have no registrar cache helper to centralize through | +| `HasPermissions::attachQueuedPermissionAssignments()` | around line 791 | Remove | saved model flush goes through `forgetModelPermissionCache()` or `forgetCachedPermissions()` | +| `HasPermissions::syncPermissions()` | around line 916 | Remove for saved models; keep only in any unsaved/queued branch that does not call a registrar helper | saved direct mutations go through `forgetModelPermissionCache()` | +| `HasPermissions::syncPermissionsWithForbidden()` | around line 962 | Keep | this is the unsaved queued branch and no registrar model-cache helper runs | +| `HasPermissions::syncPermissionsWithForbidden()` | around line 990 | Remove | saved direct mutations go through `forgetModelPermissionCache()` | +| `HasPermissions::revokePermissionTo()` | around line 1015 | Remove | saved direct mutations go through `forgetModelPermissionCache()` | +| `HasRoles::assignRole()` | around line 321 | Keep for unsaved models; remove from branches that already called `forgetModelRoleCache()` or `forgetCachedPermissions()` by restructuring the method so the direct call only runs when no cache-clearing helper ran | unsaved models only queue assignments and have no registrar cache helper to centralize through | +| `HasRoles::attachQueuedRoleAssignments()` | around line 366 | Remove | saved model flush goes through `forgetModelRoleCache()` or `forgetCachedPermissions()` | +| `HasRoles::removeRole()` | around line 407 | Remove | saved role mutations go through `forgetModelRoleCache()` | +| `HasRoles::syncRoles()` | around line 472 | Remove | saved role mutations go through `forgetModelRoleCache()`; the unsaved path delegates to `assignRole()` which keeps the direct call | + +This enumeration is intentionally specific because removing the unsaved/queued direct calls would create stale wildcard indexes within the same coroutine. + +Use the memo only in `getPermissionsViaRolesWithPivots()`: + +```php +protected function getPermissionsViaRolesWithPivots(): Collection +{ + if ($this instanceof Role || $this instanceof Permission) { + return collect(); + } + + return $this->permissionRegistrar()->rememberModelViaRolePermissions( + $this, + fn (): Collection => $this->loadPermissionsViaRolesWithPivots(), + ); +} +``` + +Move the existing materialization body into: + +```php +protected function loadPermissionsViaRolesWithPivots(): Collection +{ + $roles = $this->getCachedRoles(); + + if ($roles->isEmpty()) { + return collect(); + } + + // existing full materialization for public getters +} +``` + +Update `getPermissionsViaRoles()` to dedupe: + +```php +return $permissions + ->reject(fn (Model $permission): bool => isset($forbiddenPermissionKeys[$this->permissionComparisonKey($permission)])) + ->unique(fn (Model $permission): string => $this->permissionComparisonKey($permission)) + ->sort() + ->values(); +``` + +Also update `getAllPermissions()` to use the same comparison key: + +```php +return $directPermissions + ->merge($viaRolePermissions) + ->reject(fn (Model $permission): bool => isset($forbiddenPermissionKeys[$this->permissionComparisonKey($permission)])) + ->unique(fn (Model $permission): string => $this->permissionComparisonKey($permission)) + ->sort() + ->values(); +``` + +Use `permissionComparisonKey()` rather than bare id so same id under different guards or unusual custom models cannot collide. + +#### Invalidation Rules + +The memo is safe only with explicit invalidation: + +- `forgetCachedPermissions()` clears the whole context through `clearPermissionsCollection()` +- `forgetModelRoleCache($model)` clears the model's via-role memo +- `forgetModelAssignmentCache($model)` must clear both direct model assignment cache and via-role memo, because delete hooks use it +- role-permission changes call `forgetCachedPermissions()`, so token and catalog clearing invalidate every via-role memo +- role assignment changes call `forgetModelRoleCache($model)`, so target model memo clears immediately +- `HasAssignedModels` global bumps invalidate by token + +Do not add a direct-permission hydrated memo in this plan. Catalog indexes remove the bigger direct path cost without a second per-model hydration cache. + +#### Tests + +Add to `tests/Permission/CacheTest.php`: + +1. Warm `getPermissionsViaRoles()`. +2. Mutate the model's roles in the same coroutine. +3. Assert later `getPermissionsViaRoles()` reflects the mutation. +4. Warm `getAllPermissions()`. +5. Mutate a role's permission edges. +6. Assert later `getAllPermissions()` reflects the mutation. + +Add to `tests/Permission/ForbiddenPermissionTest.php`: + +- same permission granted through two roles appears once from `getPermissionsViaRoles()` +- forbidden duplicate remains excluded + +Run: + +```shell +./vendor/bin/phpunit --no-progress tests/Permission/CacheTest.php +./vendor/bin/phpunit --no-progress tests/Permission/ForbiddenPermissionTest.php +``` + +### 9. Remove Storage Connection Option + +#### Why + +Current config: + +```php +'storage' => [ + 'database' => [ + 'connection' => env('DB_CONNECTION', 'mysql'), + ], +], +``` + +Current migrations: + +```php +public function getConnection(): ?string +{ + return config('permission.storage.database.connection') ?: parent::getConnection(); +} +``` + +Problems: + +- config is truthy by default, so `php artisan migrate --database=...` is ignored for permission tables +- models never set the configured connection +- permission assignment tables point at app models through `model_type` and `model_id` +- split DB storage loses normal transaction coherence with users and app models +- a second DB connection adds another pool per worker +- Hypervel already has the stack cache driver for hot reads + +#### How + +Remove from `src/permission/config/permission.php`: + +```php +'storage' => [ + 'database' => [ + 'connection' => env('DB_CONNECTION', 'mysql'), + ], +], +``` + +Remove `getConnection()` from: + +- `src/permission/database/migrations/2025_07_02_000000_create_permission_tables.php` +- `src/permission/database/migrations/add_teams_fields.php.stub` + +The migrations should use normal framework behavior: + +```php +Schema::create(...); +Schema::table(...); +Schema::dropIfExists(...); +``` + +Remove the `$schema = Schema::connection($this->getConnection())` local entirely, and convert all `$schema->create(...)`, `$schema->table(...)`, `$schema->hasColumn(...)`, and `$schema->dropIfExists(...)` calls in both `up()` and `down()` to the `Schema::` facade directly. The migrator scopes the default connection for the migration body, so plain `Schema::` honors `--database` and reads as the intended framework-native design. + +Remove from `src/permission/src/Support/Config.php`: + +```php +public static function storageConnection(): ?string +{ + return self::repository()->get('permission.storage.database.connection'); +} +``` + +Remove any tests or setup lines that set `permission.storage.database.connection`, including `tests/Permission/TestCase.php`. + +Do not add a replacement config key. + +Remove the live documentation surface for this option: + +- remove the `Database Connection` ToC entry from `src/boost/docs/permission.md` +- remove the `` section from `src/boost/docs/permission.md` +- verify `src/permission/README.md` does not document a separate storage connection +- ignore historical `docs/plans/*.md` references when checking for live docs + +#### Tests + +The schema/config tests do not need source changes unless implementation reveals a failing assertion; run them as regressions after removing the config key and migration override: + +```shell +./vendor/bin/phpunit --no-progress tests/Permission/SchemaConfigTest.php +./vendor/bin/phpunit --no-progress tests/Permission/CustomSchemaConfigTest.php +``` + +Add an explicit regression assertion if the existing tests do not already cover it: permission package tables must be created on the test connection used by `migrateFreshUsing()` after the storage connection option is gone. + +Run the full permission test suite after all permission changes: + +```shell +./vendor/bin/phpunit --no-progress tests/Permission +``` + +### 10. Minor Cleanup And Docs + +#### Redundant `withPivot` + +Current: + +```php +$relation = $this->morphToMany(...)->withPivot('is_forbidden'); + +if (! Config::teamsEnabled()) { + return $relation; +} + +$teamsKey = Config::teamForeignKey(); +$relation->withPivot($teamsKey, 'is_forbidden'); +``` + +Target: + +```php +$teamsKey = Config::teamForeignKey(); +$relation->withPivot($teamsKey); +``` + +Only add the team pivot in the team branch because `is_forbidden` was already added. + +#### README And Boost Docs + +`src/boost/docs/permission.md` already says allowed getters return allowed permissions only. Add concise bullets under `Differences From Spatie Laravel Permission` in both `src/permission/README.md` and `src/boost/docs/permission.md`: + +```md +- `getDirectPermissions()`, `getPermissionsViaRoles()`, `getAllPermissions()`, and `getPermissionNames()` return effective allowed permissions. Explicit denies are exposed through `hasForbiddenPermission()` and `hasForbiddenPermissionViaRoles()`. +- Undefined `permission.cache.store` values fail fast through Hypervel's cache manager instead of silently falling back to an array store. +``` + +Do not add stale notes about removed storage connection config. The option should not appear in docs after implementation. + +#### Tests + +No separate docs tests are needed. Run the focused source tests already listed. + +## Full Verification Commands + +After all focused tests pass: + +```shell +./vendor/bin/phpstan +./vendor/bin/php-cs-fixer fix +composer test:parallel +``` + +Run these from the components repository root: + +```shell +/home/binaryfire/workspace/monorepo/contrib/hypervel/components +``` + +Do not run `vendor/bin/paratest` directly. Use `composer test:parallel`. + +## Fresh Review Checklist Before Implementation + +Before coding each item, re-read the files listed for that item. Do not rely on this plan alone. + +For every modified file: + +- check adjacent code for now-stale comments +- remove dead helpers made unnecessary by the change +- keep method placement logical +- preserve Laravel/Spatie ordering where porting order matters +- keep method docblocks title-style and concise +- avoid runtime guards that only exist for static analysis +- prefer local `@var` assertions if PHPStan needs narrowing +- run tests immediately after each test file change + +Specific review checks: + +1. Cross-guard tests cover direct user, role-via-user, and Role model paths. +2. Concrete permission object checks never become fail-open. +3. Wildcard checks still deny explicit forbidden permissions before wildcard allow. +4. `HasAssignedModels` team tests prove other teams survive assign, remove, sync, and empty sync. +5. Delete hooks do not bump global token for plain models. +6. Role/Permission deletes still bump global token through `RefreshesPermissionCache`. +7. Catalog indexes preserve `Collection` return behavior and only fast-path supported param shapes. +8. Old cache payloads without `hasForbiddenRolePermissions` are safe. +9. Via-role memo clears on same-coroutine role assignment mutation and role-permission mutation. +10. Storage connection config has zero remaining references. +11. README and Boost docs have no stale storage-connection language. + +## Expected Final State + +After this plan is implemented: + +- permission checks use guard-exact deny matching +- role-deny checks do not scan every permission for every ability +- the registrar's catalog is still the single source for role and permission lookup data in a coroutine +- indexes are internal and invalidated with the catalog +- read-side role and permission lookups use the catalog, while write-side uniqueness checks use direct DB queries +- via-role materialization is memoized only for public collection getters and invalidated centrally +- reverse assignment helpers are team-safe +- package migrations use normal framework migration connection behavior +- users who need split RBAC storage must provide custom models and migrations deliberately +- docs describe only the final supported behavior From b88460a05396ce008898a4927343d4a0526a5864 Mon Sep 17 00:00:00 2001 From: Raj Siva-Rajah <5361908+binaryfire@users.noreply.github.com> Date: Fri, 3 Jul 2026 03:28:39 +0000 Subject: [PATCH 02/14] refactor(permission): remove storage connection option 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. --- src/permission/config/permission.php | 6 ---- ..._07_02_000000_create_permission_tables.php | 32 ++++++------------- .../migrations/add_teams_fields.php.stub | 22 ++++--------- src/permission/src/Support/Config.php | 8 ----- tests/Permission/TestCase.php | 1 - 5 files changed, 16 insertions(+), 53 deletions(-) diff --git a/src/permission/config/permission.php b/src/permission/config/permission.php index 2a63998e9..97bec4613 100644 --- a/src/permission/config/permission.php +++ b/src/permission/config/permission.php @@ -30,12 +30,6 @@ 'default_model' => null, ], - 'storage' => [ - 'database' => [ - 'connection' => env('DB_CONNECTION', 'mysql'), - ], - ], - 'table_names' => [ 'roles' => 'roles', 'permissions' => 'permissions', diff --git a/src/permission/database/migrations/2025_07_02_000000_create_permission_tables.php b/src/permission/database/migrations/2025_07_02_000000_create_permission_tables.php index 84247687d..2723efa5e 100644 --- a/src/permission/database/migrations/2025_07_02_000000_create_permission_tables.php +++ b/src/permission/database/migrations/2025_07_02_000000_create_permission_tables.php @@ -7,21 +7,11 @@ use Hypervel\Support\Facades\Schema; return new class extends Migration { - /** - * Get the migration connection name. - */ - public function getConnection(): ?string - { - return config('permission.storage.database.connection') ?: parent::getConnection(); - } - /** * Run the migrations. */ public function up(): void { - $schema = Schema::connection($this->getConnection()); - $teams = (bool) config('permission.teams'); $tableNames = (array) config('permission.table_names'); $columnNames = (array) config('permission.column_names'); @@ -33,7 +23,7 @@ public function up(): void throw_if($tableNames === [], 'Error: config/permission.php not loaded. Run [php artisan config:clear] and try again.'); throw_if($teams && $teamForeignKey === '', 'Error: team_foreign_key on config/permission.php not loaded. Run [php artisan config:clear] and try again.'); - $schema->create($tableNames['permissions'], static function (Blueprint $table): void { + Schema::create($tableNames['permissions'], static function (Blueprint $table): void { $table->id(); $table->string('name'); $table->string('guard_name'); @@ -42,7 +32,7 @@ public function up(): void $table->unique(['name', 'guard_name']); }); - $schema->create($tableNames['roles'], static function (Blueprint $table) use ($teams, $teamForeignKey): void { + Schema::create($tableNames['roles'], static function (Blueprint $table) use ($teams, $teamForeignKey): void { $table->id(); if ($teams) { @@ -61,7 +51,7 @@ public function up(): void } }); - $schema->create($tableNames['model_has_permissions'], static function (Blueprint $table) use ($tableNames, $modelMorphKey, $pivotPermission, $teams, $teamForeignKey): void { + Schema::create($tableNames['model_has_permissions'], static function (Blueprint $table) use ($tableNames, $modelMorphKey, $pivotPermission, $teams, $teamForeignKey): void { $table->unsignedBigInteger($pivotPermission); $table->string('model_type'); $table->unsignedBigInteger($modelMorphKey); @@ -83,7 +73,7 @@ public function up(): void } }); - $schema->create($tableNames['model_has_roles'], static function (Blueprint $table) use ($tableNames, $modelMorphKey, $pivotRole, $teams, $teamForeignKey): void { + Schema::create($tableNames['model_has_roles'], static function (Blueprint $table) use ($tableNames, $modelMorphKey, $pivotRole, $teams, $teamForeignKey): void { $table->unsignedBigInteger($pivotRole); $table->string('model_type'); $table->unsignedBigInteger($modelMorphKey); @@ -104,7 +94,7 @@ public function up(): void } }); - $schema->create($tableNames['role_has_permissions'], static function (Blueprint $table) use ($tableNames, $pivotRole, $pivotPermission): void { + Schema::create($tableNames['role_has_permissions'], static function (Blueprint $table) use ($tableNames, $pivotRole, $pivotPermission): void { $table->unsignedBigInteger($pivotPermission); $table->unsignedBigInteger($pivotRole); $table->boolean('is_forbidden')->default(false); @@ -132,16 +122,14 @@ public function up(): void */ public function down(): void { - $schema = Schema::connection($this->getConnection()); - $tableNames = (array) config('permission.table_names'); throw_if($tableNames === [], 'Error: config/permission.php not found and defaults could not be merged. Please publish the package configuration before proceeding, or drop the tables manually.'); - $schema->dropIfExists($tableNames['role_has_permissions']); - $schema->dropIfExists($tableNames['model_has_roles']); - $schema->dropIfExists($tableNames['model_has_permissions']); - $schema->dropIfExists($tableNames['roles']); - $schema->dropIfExists($tableNames['permissions']); + Schema::dropIfExists($tableNames['role_has_permissions']); + Schema::dropIfExists($tableNames['model_has_roles']); + Schema::dropIfExists($tableNames['model_has_permissions']); + Schema::dropIfExists($tableNames['roles']); + Schema::dropIfExists($tableNames['permissions']); } }; diff --git a/src/permission/database/migrations/add_teams_fields.php.stub b/src/permission/database/migrations/add_teams_fields.php.stub index 8ec714088..684ff5a46 100644 --- a/src/permission/database/migrations/add_teams_fields.php.stub +++ b/src/permission/database/migrations/add_teams_fields.php.stub @@ -8,21 +8,11 @@ use Hypervel\Support\Facades\DB; use Hypervel\Support\Facades\Schema; return new class extends Migration { - /** - * Get the migration connection name. - */ - public function getConnection(): ?string - { - return config('permission.storage.database.connection') ?: parent::getConnection(); - } - /** * Run the migrations. */ public function up(): void { - $schema = Schema::connection($this->getConnection()); - $teams = (bool) config('permission.teams'); $tableNames = (array) config('permission.table_names'); $columnNames = (array) config('permission.column_names'); @@ -38,8 +28,8 @@ return new class extends Migration { throw_if($tableNames === [], 'Error: config/permission.php not loaded. Run [php artisan config:clear] and try again.'); throw_if($teamForeignKey === '', 'Error: team_foreign_key on config/permission.php not loaded. Run [php artisan config:clear] and try again.'); - if (! $schema->hasColumn($tableNames['roles'], $teamForeignKey)) { - $schema->table($tableNames['roles'], static function (Blueprint $table) use ($teamForeignKey): void { + if (! Schema::hasColumn($tableNames['roles'], $teamForeignKey)) { + Schema::table($tableNames['roles'], static function (Blueprint $table) use ($teamForeignKey): void { $table->unsignedBigInteger($teamForeignKey)->nullable()->after('id'); $table->index($teamForeignKey, 'roles_team_foreign_key_index'); @@ -48,8 +38,8 @@ return new class extends Migration { }); } - if (! $schema->hasColumn($tableNames['model_has_permissions'], $teamForeignKey)) { - $schema->table($tableNames['model_has_permissions'], static function (Blueprint $table) use ($tableNames, $teamForeignKey, $modelMorphKey, $pivotPermission): void { + if (! Schema::hasColumn($tableNames['model_has_permissions'], $teamForeignKey)) { + Schema::table($tableNames['model_has_permissions'], static function (Blueprint $table) use ($tableNames, $teamForeignKey, $modelMorphKey, $pivotPermission): void { $table->unsignedBigInteger($teamForeignKey)->default(1); $table->index($teamForeignKey, 'model_has_permissions_team_foreign_key_index'); @@ -70,8 +60,8 @@ return new class extends Migration { }); } - if (! $schema->hasColumn($tableNames['model_has_roles'], $teamForeignKey)) { - $schema->table($tableNames['model_has_roles'], static function (Blueprint $table) use ($tableNames, $teamForeignKey, $modelMorphKey, $pivotRole): void { + if (! Schema::hasColumn($tableNames['model_has_roles'], $teamForeignKey)) { + Schema::table($tableNames['model_has_roles'], static function (Blueprint $table) use ($tableNames, $teamForeignKey, $modelMorphKey, $pivotRole): void { $table->unsignedBigInteger($teamForeignKey)->default(1); $table->index($teamForeignKey, 'model_has_roles_team_foreign_key_index'); diff --git a/src/permission/src/Support/Config.php b/src/permission/src/Support/Config.php index 330641e5b..0d51e0c7b 100644 --- a/src/permission/src/Support/Config.php +++ b/src/permission/src/Support/Config.php @@ -104,14 +104,6 @@ public static function permissionsTable(): string return self::repository()->string('permission.table_names.permissions'); } - /** - * Get the configured storage connection. - */ - public static function storageConnection(): ?string - { - return self::repository()->get('permission.storage.database.connection'); - } - /** * Get the model morph key column. */ diff --git a/tests/Permission/TestCase.php b/tests/Permission/TestCase.php index 2f1a23b1e..645bb9f40 100644 --- a/tests/Permission/TestCase.php +++ b/tests/Permission/TestCase.php @@ -70,7 +70,6 @@ protected function defineEnvironment(ApplicationContract $app): void $app->make('config')->set([ 'database.default' => 'testing', - 'permission.storage.database.connection' => 'testing', 'permission.register_permission_check_method' => true, 'permission.teams' => false, 'permission.column_names.model_morph_key' => 'model_test_id', From 087d74d5efcd185e9de7e78994807c05486095f2 Mon Sep 17 00:00:00 2001 From: Raj Siva-Rajah <5361908+binaryfire@users.noreply.github.com> Date: Fri, 3 Jul 2026 03:28:45 +0000 Subject: [PATCH 03/14] fix(permission): handle authenticated users without authorizable 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. --- .../src/Exceptions/UnauthorizedException.php | 5 ++--- .../Models/PlainAuthenticatableUser.php | 20 +++++++++++++++++++ .../Middleware/PermissionMiddlewareTest.php | 8 ++++++++ .../Middleware/RoleMiddlewareTest.php | 8 ++++++++ .../RoleOrPermissionMiddlewareTest.php | 8 ++++++++ 5 files changed, 46 insertions(+), 3 deletions(-) create mode 100644 tests/Permission/Fixtures/Models/PlainAuthenticatableUser.php diff --git a/src/permission/src/Exceptions/UnauthorizedException.php b/src/permission/src/Exceptions/UnauthorizedException.php index 017090fb1..9ae672eb8 100644 --- a/src/permission/src/Exceptions/UnauthorizedException.php +++ b/src/permission/src/Exceptions/UnauthorizedException.php @@ -4,7 +4,6 @@ namespace Hypervel\Permission\Exceptions; -use Hypervel\Contracts\Auth\Access\Authorizable; use Hypervel\Permission\Support\Config; use Symfony\Component\HttpKernel\Exception\HttpException; @@ -84,9 +83,9 @@ public static function forRolesOrPermissions(array $rolesOrPermissions): static /** * Create an exception for a user missing the HasRoles trait. */ - public static function missingTraitHasRoles(Authorizable $user): static + public static function missingTraitHasRoles(object $user): static { - return new static(403, __('Authorizable class `:class` must use Hypervel\Permission\Traits\HasRoles trait.', [ + return new static(403, __('Authenticated class `:class` must use Hypervel\Permission\Traits\HasRoles trait.', [ 'class' => $user::class, ]), null, []); } diff --git a/tests/Permission/Fixtures/Models/PlainAuthenticatableUser.php b/tests/Permission/Fixtures/Models/PlainAuthenticatableUser.php new file mode 100644 index 000000000..f089d2a8f --- /dev/null +++ b/tests/Permission/Fixtures/Models/PlainAuthenticatableUser.php @@ -0,0 +1,20 @@ +assertSame(403, $this->runMiddleware($this->permissionMiddleware, 'edit-news')); } + public function testPlainAuthenticatableUserWithoutAuthorizableCannotAccessRoute(): void + { + Auth::login(PlainAuthenticatableUser::create(['email' => 'plain_authenticatable@user.com'])); + + $this->assertSame(403, $this->runMiddleware($this->permissionMiddleware, 'edit-news')); + } + public function testGuardSpecificPermissionIsUsed(): void { $this->app->make(Permission::class)->create(['name' => 'admin-permission2', 'guard_name' => 'web']); diff --git a/tests/Permission/Middleware/RoleMiddlewareTest.php b/tests/Permission/Middleware/RoleMiddlewareTest.php index d4442d0d9..532d5a61e 100644 --- a/tests/Permission/Middleware/RoleMiddlewareTest.php +++ b/tests/Permission/Middleware/RoleMiddlewareTest.php @@ -10,6 +10,7 @@ use Hypervel\Permission\Exceptions\UnauthorizedException; use Hypervel\Permission\Middleware\RoleMiddleware; use Hypervel\Support\Facades\Auth; +use Hypervel\Tests\Permission\Fixtures\Models\PlainAuthenticatableUser; use Hypervel\Tests\Permission\Fixtures\Models\TestRolePermissionsEnum; use Hypervel\Tests\Permission\Fixtures\Models\UserWithoutHasRoles; use Hypervel\Tests\Permission\TestCase; @@ -89,6 +90,13 @@ public function testUserWithoutHasRolesTraitCannotAccessRoute(): void $this->assertSame(403, $this->runMiddleware($this->roleMiddleware, 'testRole')); } + public function testPlainAuthenticatableUserWithoutAuthorizableCannotAccessRoute(): void + { + Auth::login(PlainAuthenticatableUser::create(['email' => 'plain_authenticatable@user.com'])); + + $this->assertSame(403, $this->runMiddleware($this->roleMiddleware, 'testRole')); + } + public function testUserCanAccessRoleWithMatchingGuard(): void { Auth::guard('admin')->login($this->testAdmin); diff --git a/tests/Permission/Middleware/RoleOrPermissionMiddlewareTest.php b/tests/Permission/Middleware/RoleOrPermissionMiddlewareTest.php index 5eb548280..75b39c44f 100644 --- a/tests/Permission/Middleware/RoleOrPermissionMiddlewareTest.php +++ b/tests/Permission/Middleware/RoleOrPermissionMiddlewareTest.php @@ -10,6 +10,7 @@ use Hypervel\Permission\Middleware\RoleOrPermissionMiddleware; use Hypervel\Support\Facades\Auth; use Hypervel\Support\Facades\Gate; +use Hypervel\Tests\Permission\Fixtures\Models\PlainAuthenticatableUser; use Hypervel\Tests\Permission\Fixtures\Models\UserWithoutHasRoles; use Hypervel\Tests\Permission\TestCase; use InvalidArgumentException; @@ -65,6 +66,13 @@ public function testUserWithoutHasRolesTraitCannotAccessRoute(): void $this->assertSame(403, $this->runMiddleware($this->roleOrPermissionMiddleware, 'testRole|edit-articles')); } + public function testPlainAuthenticatableUserWithoutAuthorizableCannotAccessRoute(): void + { + Auth::login(PlainAuthenticatableUser::create(['email' => 'plain_authenticatable@user.com'])); + + $this->assertSame(403, $this->runMiddleware($this->roleOrPermissionMiddleware, 'testRole|edit-articles')); + } + public function testUserCannotAccessRouteWithoutMatchingPermissionOrRole(): void { Auth::login($this->testUser); From 96ffae6559b7190abe27e83b9e454404f893e7d6 Mon Sep 17 00:00:00 2001 From: Raj Siva-Rajah <5361908+binaryfire@users.noreply.github.com> Date: Fri, 3 Jul 2026 03:29:05 +0000 Subject: [PATCH 04/14] fix(permission): parse malformed pipe role strings correctly 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. --- src/permission/src/Traits/HasRoles.php | 2 +- tests/Permission/Traits/HasRolesTest.php | 9 +++++++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/src/permission/src/Traits/HasRoles.php b/src/permission/src/Traits/HasRoles.php index 7563b9182..849173a69 100644 --- a/src/permission/src/Traits/HasRoles.php +++ b/src/permission/src/Traits/HasRoles.php @@ -650,7 +650,7 @@ protected function convertPipeToArray(string $pipeString): array } $quoteCharacter = substr($pipeString, 0, 1); - $endCharacter = substr($quoteCharacter, -1, 1); + $endCharacter = substr($pipeString, -1, 1); if ($quoteCharacter !== $endCharacter) { return explode('|', $pipeString); diff --git a/tests/Permission/Traits/HasRolesTest.php b/tests/Permission/Traits/HasRolesTest.php index a6112bbf2..5e8e15dd6 100644 --- a/tests/Permission/Traits/HasRolesTest.php +++ b/tests/Permission/Traits/HasRolesTest.php @@ -174,6 +174,15 @@ public function testItCanAssignAndRemoveARoleUsingAnId(): void $this->assertFalse($this->testUser->hasRole($this->testUserRole)); } + public function testMalformedQuotedPipeRoleStringDoesNotTrimLeadingQuote(): void + { + app(Role::class)->create(['name' => 'admin']); + + $this->testUser->assignRole('admin'); + + $this->assertFalse($this->testUser->hasRole('"admin|editor')); + } + public function testItCanAssignAndRemoveMultipleRolesAtOnce(): void { $this->testUser->assignRole($this->testUserRole->getKey(), 'testRole2'); From 3f6c62bc303c7cb59e88a7967c35c5b2f8003596 Mon Sep 17 00:00:00 2001 From: Raj Siva-Rajah <5361908+binaryfire@users.noreply.github.com> Date: Fri, 3 Jul 2026 03:29:14 +0000 Subject: [PATCH 05/14] fix(permission): scope reverse role assignments by team 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. --- .../src/Traits/HasAssignedModels.php | 16 ++- .../Traits/TeamHasAssignedModelsTest.php | 133 ++++++++++++++++++ 2 files changed, 147 insertions(+), 2 deletions(-) create mode 100644 tests/Permission/Traits/TeamHasAssignedModelsTest.php diff --git a/src/permission/src/Traits/HasAssignedModels.php b/src/permission/src/Traits/HasAssignedModels.php index 426a3f01f..b53dae590 100644 --- a/src/permission/src/Traits/HasAssignedModels.php +++ b/src/permission/src/Traits/HasAssignedModels.php @@ -94,13 +94,19 @@ public function syncModels(array|Collection|Model|int|string $models, ?string $m */ protected function relationForModel(string $modelClass): MorphToMany { - return $this->morphedByMany( + $relation = $this->morphedByMany( $modelClass, 'model', Config::modelHasRolesTable(), Container::getInstance()->make(PermissionRegistrar::class)->pivotRole, Config::morphKey(), ); + + if (! Config::teamsEnabled()) { + return $relation; + } + + return $relation->wherePivot(Config::teamForeignKey(), getPermissionsTeamId()); } /** @@ -155,8 +161,14 @@ private function teamPivot(): array private function newPivotQueryForRole(): Builder { - return $this->getConnection() + $query = $this->getConnection() ->table(Config::modelHasRolesTable()) ->where(Container::getInstance()->make(PermissionRegistrar::class)->pivotRole, $this->getKey()); + + if (Config::teamsEnabled()) { + $query->where(Config::teamForeignKey(), getPermissionsTeamId()); + } + + return $query; } } diff --git a/tests/Permission/Traits/TeamHasAssignedModelsTest.php b/tests/Permission/Traits/TeamHasAssignedModelsTest.php new file mode 100644 index 000000000..f1b33f091 --- /dev/null +++ b/tests/Permission/Traits/TeamHasAssignedModelsTest.php @@ -0,0 +1,133 @@ +make('config')->set('permission.teams', true); + } + + protected function setUpInCoroutine(): void + { + $this->setUpTeams(); + } + + public function testItAssignsModelsInCurrentTeamWhenModelAlreadyHasRoleInAnotherTeam(): void + { + $user = User::create(['email' => 'user1@test.com']); + + setPermissionsTeamId(2); + $user->assignRole($this->testUserRole); + + setPermissionsTeamId(1); + $this->testUserRole->assignToModels($user); + + $this->assertSame(2, $this->roleAssignmentsFor($user)); + + setPermissionsTeamId(1); + $this->assertTrue($user->fresh()->hasRole($this->testUserRole)); + + setPermissionsTeamId(2); + $this->assertTrue($user->fresh()->hasRole($this->testUserRole)); + } + + public function testItRemovesModelsOnlyFromCurrentTeam(): void + { + $user = User::create(['email' => 'user1@test.com']); + + setPermissionsTeamId(1); + $user->assignRole($this->testUserRole); + + setPermissionsTeamId(2); + $user->assignRole($this->testUserRole); + + setPermissionsTeamId(1); + $this->testUserRole->removeFromModels($user); + + $this->assertSame(1, $this->roleAssignmentsFor($user)); + $this->assertFalse($user->fresh()->hasRole($this->testUserRole)); + + setPermissionsTeamId(2); + $this->assertTrue($user->fresh()->hasRole($this->testUserRole)); + } + + public function testItSyncsModelsOnlyInCurrentTeam(): void + { + $user1 = User::create(['email' => 'user1@test.com']); + $user2 = User::create(['email' => 'user2@test.com']); + $user3 = User::create(['email' => 'user3@test.com']); + + setPermissionsTeamId(1); + $user1->assignRole($this->testUserRole); + + setPermissionsTeamId(2); + $user3->assignRole($this->testUserRole); + + setPermissionsTeamId(1); + $this->testUserRole->syncModels([$user2]); + + $this->assertFalse($user1->fresh()->hasRole($this->testUserRole)); + $this->assertTrue($user2->fresh()->hasRole($this->testUserRole)); + + setPermissionsTeamId(2); + $this->assertTrue($user3->fresh()->hasRole($this->testUserRole)); + } + + public function testItClearsModelsOnlyInCurrentTeamWhenSyncingEmptyList(): void + { + $user1 = User::create(['email' => 'user1@test.com']); + $user2 = User::create(['email' => 'user2@test.com']); + + setPermissionsTeamId(1); + $user1->assignRole($this->testUserRole); + + setPermissionsTeamId(2); + $user2->assignRole($this->testUserRole); + + setPermissionsTeamId(1); + $this->testUserRole->syncModels([]); + + $this->assertFalse($user1->fresh()->hasRole($this->testUserRole)); + + setPermissionsTeamId(2); + $this->assertTrue($user2->fresh()->hasRole($this->testUserRole)); + } + + public function testRawIdsWithExplicitModelClassHonorCurrentTeam(): void + { + $user = User::create(['email' => 'user1@test.com']); + + setPermissionsTeamId(2); + $this->testUserRole->assignToModels($user->getKey(), User::class); + + setPermissionsTeamId(1); + $this->testUserRole->assignToModels($user->getKey(), User::class); + $this->testUserRole->removeFromModels($user->getKey(), User::class); + + $this->assertSame(1, $this->roleAssignmentsFor($user)); + $this->assertFalse($user->fresh()->hasRole($this->testUserRole)); + + setPermissionsTeamId(2); + $this->assertTrue($user->fresh()->hasRole($this->testUserRole)); + } + + private function roleAssignmentsFor(User $user): int + { + return DB::table(Config::modelHasRolesTable()) + ->where(app(PermissionRegistrar::class)->pivotRole, $this->testUserRole->getKey()) + ->where(Config::morphKey(), $user->getKey()) + ->count(); + } +} From f5c0ef1ea0ede814fb63f89abe81445dc439f64d Mon Sep 17 00:00:00 2001 From: Raj Siva-Rajah <5361908+binaryfire@users.noreply.github.com> Date: Fri, 3 Jul 2026 03:30:01 +0000 Subject: [PATCH 06/14] perf(permission): avoid redundant role assignment cache clears 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. --- src/permission/src/Traits/HasRoles.php | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/permission/src/Traits/HasRoles.php b/src/permission/src/Traits/HasRoles.php index 849173a69..af7d4ba1f 100644 --- a/src/permission/src/Traits/HasRoles.php +++ b/src/permission/src/Traits/HasRoles.php @@ -61,9 +61,9 @@ public static function bootHasRoles(): void ->where(Config::morphKey(), $model->getKey()) ->where('model_type', $model->getMorphClass()) ->delete(); - } - $registrar->bumpModelAssignmentCacheToken(); + $registrar->forgetModelAssignmentCache($model); + } }); static::saved(function (Model $model): void { @@ -297,6 +297,7 @@ public function assignRole(...$roles): static $registrar = $this->permissionRegistrar(); $teamPivot = $registrar->teams && ! $this instanceof Permission ? [$registrar->teamsKey => getPermissionsTeamId()] : []; + $cacheCleared = false; if ($model->exists) { if ($registrar->teams) { @@ -314,11 +315,15 @@ public function assignRole(...$roles): static if ($this instanceof Permission) { $this->forgetCachedPermissions(); + $cacheCleared = true; } elseif ($model->exists) { $registrar->forgetModelRoleCache($model); + $cacheCleared = true; } - $this->forgetWildcardPermissionIndex(); + if (! $cacheCleared) { + $this->forgetWildcardPermissionIndex(); + } $this->dispatchRoleAttachedEvent($roles); @@ -362,8 +367,6 @@ protected function attachQueuedRoleAssignments(): void } else { $registrar->forgetModelRoleCache($this); } - - $this->forgetWildcardPermissionIndex(); } /** @@ -404,8 +407,6 @@ public function removeRole(...$role): static $this->permissionRegistrar()->forgetModelRoleCache($this); } - $this->forgetWildcardPermissionIndex(); - $this->dispatchRoleDetachedEvent($roles); return $this; @@ -469,7 +470,6 @@ public function syncRoles(...$roles): static $registrar->forgetModelRoleCache($this); } - $this->forgetWildcardPermissionIndex(); $this->dispatchRoleAttachedEvent($roles); return $this; From 7ede82e7e493b8700faa48d2d61526b125a999e3 Mon Sep 17 00:00:00 2001 From: Raj Siva-Rajah <5361908+binaryfire@users.noreply.github.com> Date: Fri, 3 Jul 2026 03:30:08 +0000 Subject: [PATCH 07/14] perf(permission): index registrar catalog and runtime caches 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. --- src/permission/src/PermissionRegistrar.php | 325 ++++++++++++++++++--- 1 file changed, 289 insertions(+), 36 deletions(-) diff --git a/src/permission/src/PermissionRegistrar.php b/src/permission/src/PermissionRegistrar.php index 35c058e70..0b38e9bba 100644 --- a/src/permission/src/PermissionRegistrar.php +++ b/src/permission/src/PermissionRegistrar.php @@ -23,9 +23,12 @@ use Hypervel\Permission\Models\Permission; use Hypervel\Permission\Models\Role; use Hypervel\Support\Arr; +use Hypervel\Support\Collection as BaseCollection; use Hypervel\Support\Str; use InvalidArgumentException; +use function Hypervel\Support\enum_value; + class PermissionRegistrar { public const MODEL_ROLES_CACHE_KEY_PREFIX = 'hypervel.permission.cache.model.roles'; @@ -38,6 +41,8 @@ class PermissionRegistrar public const WILDCARD_PERMISSION_INDEX_CONTEXT_KEY = '__permission.wildcard_index'; + public const MODEL_VIA_ROLE_PERMISSIONS_CONTEXT_KEY = '__permission.model_via_role_permissions'; + /** * The callback used to scope permission cache keys. */ @@ -221,6 +226,9 @@ public function forgetModelAssignmentCache(Model $model): void $cache->forget($this->modelCacheKey($this->modelRolesCacheKeyPrefix, $model)); $cache->forget($this->modelCacheKey($this->modelPermissionsCacheKeyPrefix, $model)); + + $this->forgetModelViaRolePermissions($model); + $this->forgetWildcardPermissionIndex($model); } /** @@ -231,6 +239,9 @@ public function forgetModelRoleCache(Model $model): void $this->cacheRepository()->forget( $this->modelCacheKey($this->modelRolesCacheKeyPrefix, $model) ); + + $this->forgetModelViaRolePermissions($model); + $this->forgetWildcardPermissionIndex($model); } /** @@ -241,6 +252,38 @@ public function forgetModelPermissionCache(Model $model): void $this->cacheRepository()->forget( $this->modelCacheKey($this->modelPermissionsCacheKeyPrefix, $model) ); + + $this->forgetWildcardPermissionIndex($model); + } + + /** + * Remember a model's permissions granted through roles. + * + * @param Closure(): BaseCollection $callback + */ + public function rememberModelViaRolePermissions(Model $model, Closure $callback): BaseCollection + { + $key = $this->modelRuntimeCacheKey($model); + $items = CoroutineContext::get(self::MODEL_VIA_ROLE_PERMISSIONS_CONTEXT_KEY, []); + + if (isset($items[$key]) && $items[$key] instanceof BaseCollection) { + return $items[$key]; + } + + $items[$key] = $callback(); + CoroutineContext::set(self::MODEL_VIA_ROLE_PERMISSIONS_CONTEXT_KEY, $items); + + return $items[$key]; + } + + /** + * Forget a model's permissions granted through roles. + */ + public function forgetModelViaRolePermissions(Model $model): void + { + $items = CoroutineContext::get(self::MODEL_VIA_ROLE_PERMISSIONS_CONTEXT_KEY, []); + unset($items[$this->modelRuntimeCacheKey($model)]); + CoroutineContext::set(self::MODEL_VIA_ROLE_PERMISSIONS_CONTEXT_KEY, $items); } /** @@ -289,6 +332,28 @@ protected function modelCacheKey(string $prefix, Model $model): string ]); } + /** + * Build the runtime cache key segment for coroutine-local model state. + */ + protected function modelRuntimeCacheKey(Model $model): string + { + $teamId = $this->teams ? (string) ($this->getPermissionsTeamId() ?? 'global') : 'none'; + $segments = [ + $this->modelAssignmentCacheToken(), + $model->getMorphClass(), + $model->getKey(), + $teamId, + ]; + + $scope = $this->cacheKeyScopeSegment(); + + if ($scope !== '') { + array_unshift($segments, $scope); + } + + return implode(':', $segments); + } + /** * Get the current model assignment cache token. */ @@ -370,21 +435,7 @@ public function getWildcardPermissionIndex(Model $record): array */ protected function wildcardPermissionIndexKey(Model $record): string { - $teamId = $this->teams ? (string) ($this->getPermissionsTeamId() ?? 'global') : 'none'; - $segments = [ - $this->modelAssignmentCacheToken(), - $record->getMorphClass(), - $record->getKey(), - $teamId, - ]; - - $scope = $this->cacheKeyScopeSegment(); - - if ($scope !== '') { - array_unshift($segments, $scope); - } - - return implode(':', $segments); + return $this->modelRuntimeCacheKey($record); } /** @@ -394,12 +445,23 @@ public function clearPermissionsCollection(): void { CoroutineContext::forget(self::PERMISSION_CATALOG_CONTEXT_KEY); CoroutineContext::forget(self::WILDCARD_PERMISSION_INDEX_CONTEXT_KEY); + CoroutineContext::forget(self::MODEL_VIA_ROLE_PERMISSIONS_CONTEXT_KEY); } /** * Get the hydrated permission catalog. * - * @return array{permissions: Collection, roles: Collection} + * @return array{ + * permissions: Collection, + * roles: Collection, + * permissionByKey: array, + * permissionByNameAndGuard: array>, + * permissionOrderByKey: array, + * roleByKey: array, + * roleByNameAndGuard: array>, + * roleOrderByKey: array, + * hasForbiddenRolePermissions: bool + * } */ private function permissionCatalog(): array { @@ -407,11 +469,11 @@ private function permissionCatalog(): array $catalogs = CoroutineContext::get(self::PERMISSION_CATALOG_CONTEXT_KEY, []); if (isset($catalogs[$contextKey]) && is_array($catalogs[$contextKey])) { - /** @var array{permissions: Collection, roles: Collection} */ + /** @var array{permissions: Collection, roles: Collection, permissionByKey: array, permissionByNameAndGuard: array>, permissionOrderByKey: array, roleByKey: array, roleByNameAndGuard: array>, roleOrderByKey: array, hasForbiddenRolePermissions: bool} */ return $catalogs[$contextKey]; } - /** @var array{permissions: array>, roles: array>} $payload */ + /** @var array{permissions: array>, roles: array>, hasForbiddenRolePermissions?: bool} $payload */ $payload = $this->cacheRepository()->remember( $contextKey, $this->cacheExpirationTime, @@ -419,10 +481,18 @@ private function permissionCatalog(): array ); $roles = $this->getHydratedRoleCollection($payload['roles']); + $permissions = $this->getHydratedPermissionCollection($payload['permissions'], $roles); $catalog = [ 'roles' => $roles, - 'permissions' => $this->getHydratedPermissionCollection($payload['permissions'], $roles), + 'permissions' => $permissions, + 'permissionByKey' => $this->indexModelsByKey($permissions), + 'permissionByNameAndGuard' => $this->indexModelsByNameAndGuard($permissions), + 'permissionOrderByKey' => $this->indexModelOrderByKey($permissions), + 'roleByKey' => $this->indexModelsByKey($roles), + 'roleByNameAndGuard' => $this->indexModelsByNameAndGuard($roles), + 'roleOrderByKey' => $this->indexModelOrderByKey($roles), + 'hasForbiddenRolePermissions' => (bool) ($payload['hasForbiddenRolePermissions'] ?? false), ]; $catalogs[$contextKey] = $catalog; @@ -446,9 +516,13 @@ public function getPermissions(array $params = [], bool $onlyOne = false, ?strin ); } - $permissions = $this->permissionCatalog()['permissions']; + $indexed = $this->indexedModels($params, $onlyOne, 'permission'); - return $this->filterModels($permissions, $params, $onlyOne); + if ($indexed !== null) { + return $indexed; + } + + return $this->filterModels($this->permissionCatalog()['permissions'], $params, $onlyOne); } /** @@ -466,9 +540,116 @@ public function getRoles(array $params = [], bool $onlyOne = false, ?string $rol ); } - $roles = $this->permissionCatalog()['roles']; + $indexed = $this->indexedModels($params, $onlyOne, 'role'); - return $this->filterModels($roles, $params, $onlyOne); + if ($indexed !== null) { + return $indexed; + } + + return $this->filterModels($this->permissionCatalog()['roles'], $params, $onlyOne); + } + + /** + * Get indexed models for supported exact lookup shapes. + * + * @param array $params + */ + protected function indexedModels(array $params, bool $onlyOne, string $modelType): ?Collection + { + if ($params === []) { + return null; + } + + $catalog = $this->permissionCatalog(); + $keyName = $modelType === 'permission' + ? Guard::getModelKeyName($this->permissionClass) + : Guard::getModelKeyName($this->roleClass); + + /** @var array $byKey */ + $byKey = $catalog[$modelType === 'permission' ? 'permissionByKey' : 'roleByKey']; + /** @var array> $byNameAndGuard */ + $byNameAndGuard = $catalog[$modelType === 'permission' ? 'permissionByNameAndGuard' : 'roleByNameAndGuard']; + /** @var array $orderByKey */ + $orderByKey = $catalog[$modelType === 'permission' ? 'permissionOrderByKey' : 'roleOrderByKey']; + + if (array_key_exists($keyName, $params) && count(array_diff_key($params, [$keyName => true, 'guard_name' => true])) === 0) { + $ids = Arr::wrap($params[$keyName]); + $guardNames = array_key_exists('guard_name', $params) ? Arr::wrap($params['guard_name']) : null; + $matches = []; + + foreach ($ids as $id) { + $model = $byKey[(string) $id] ?? null; + + if (! $model instanceof Model) { + continue; + } + + if ($guardNames !== null && ! self::attributeMatches($model->getAttribute('guard_name'), $guardNames)) { + continue; + } + + if ($modelType === 'role' && ! $this->roleMatchesCurrentTeam($model)) { + continue; + } + + $matches[(string) $model->getKey()] = $model; + } + + return $this->collectionFromIndexedModels($matches, $orderByKey, $onlyOne); + } + + if (array_key_exists('name', $params) && array_key_exists('guard_name', $params) && count($params) === 2) { + $names = Arr::wrap($params['name']); + $guards = Arr::wrap($params['guard_name']); + $matches = []; + + foreach ($guards as $guardName) { + foreach ($names as $name) { + foreach ($byNameAndGuard[$this->nameGuardIndexKey($name, $guardName)] ?? [] as $model) { + if ($modelType === 'role' && ! $this->roleMatchesCurrentTeam($model)) { + continue; + } + + $matches[(string) $model->getKey()] = $model; + } + } + } + + return $this->collectionFromIndexedModels($matches, $orderByKey, $onlyOne); + } + + return null; + } + + /** + * Determine if a cached role matches the current team scope. + */ + private function roleMatchesCurrentTeam(Model $role): bool + { + if (! $this->teams) { + return true; + } + + $teamId = $this->getPermissionsTeamId(); + $roleTeamId = $role->getAttribute($this->teamsKey); + + return $roleTeamId === null || self::attributeMatches($roleTeamId, $teamId); + } + + /** + * Build a collection from indexed models in catalog order. + * + * @param array $models + * @param array $orderByKey + */ + private function collectionFromIndexedModels(array $models, array $orderByKey, bool $onlyOne): Collection + { + uasort($models, fn (Model $a, Model $b): int => ($orderByKey[(string) $a->getKey()] ?? PHP_INT_MAX) + <=> ($orderByKey[(string) $b->getKey()] ?? PHP_INT_MAX)); + + $collection = new Collection(array_values($models)); + + return $onlyOne ? $collection->take(1)->values() : $collection->values(); } /** @@ -674,27 +855,37 @@ protected function getRolesForCache(): Collection /** * Serialize permissions for cache. * - * @return array{permissions: array>, roles: array>} + * @return array{permissions: array>, roles: array>, hasForbiddenRolePermissions: bool} */ private function getSerializedPermissionsForCache(): array { $except = $this->config->array('permission.cache.column_names_except', ['created_at', 'updated_at', 'deleted_at']); + $hasForbiddenRolePermissions = false; return [ 'permissions' => $this->getPermissionsWithRoles() - ->map(fn (Model $permission): array => [ - 'attributes' => Arr::except($permission->getAttributes(), $except), - 'roles' => $this->relationCollection($permission, 'roles') - ->map(fn (Model $role): array => [ - 'pivot' => [ - $this->pivotPermission => $permission->getKey(), - $this->pivotRole => $role->getKey(), - 'is_forbidden' => $this->pivotIsForbidden($role), - ], - ]) + ->map(function (Model $permission) use ($except, &$hasForbiddenRolePermissions): array { + $roles = $this->relationCollection($permission, 'roles') + ->map(function (Model $role) use ($permission, &$hasForbiddenRolePermissions): array { + $isForbidden = $this->pivotIsForbidden($role); + $hasForbiddenRolePermissions = $hasForbiddenRolePermissions || $isForbidden; + + return [ + 'pivot' => [ + $this->pivotPermission => $permission->getKey(), + $this->pivotRole => $role->getKey(), + 'is_forbidden' => $isForbidden, + ], + ]; + }) ->values() - ->all(), - ]) + ->all(); + + return [ + 'attributes' => Arr::except($permission->getAttributes(), $except), + 'roles' => $roles, + ]; + }) ->values() ->all(), 'roles' => $this->getRolesForCache() @@ -703,9 +894,18 @@ private function getSerializedPermissionsForCache(): array ]) ->values() ->all(), + 'hasForbiddenRolePermissions' => $hasForbiddenRolePermissions, ]; } + /** + * Determine if any cached role-permission edge is forbidden. + */ + public function hasForbiddenRolePermissions(): bool + { + return (bool) $this->permissionCatalog()['hasForbiddenRolePermissions']; + } + /** * Determine if a hydrated pivot marks the permission as forbidden. */ @@ -768,6 +968,59 @@ private function getHydratedRoleCollection(array $roles): Collection )); } + /** + * Index models by primary key. + * + * @return array + */ + private function indexModelsByKey(Collection $models): array + { + return $models + ->mapWithKeys(fn (Model $model): array => [(string) $model->getKey() => $model]) + ->all(); + } + + /** + * Index models by name and guard. + * + * @return array> + */ + private function indexModelsByNameAndGuard(Collection $models): array + { + $indexed = []; + + foreach ($models as $model) { + $indexed[$this->nameGuardIndexKey($model->getAttribute('name'), $model->getAttribute('guard_name'))][] = $model; + } + + return $indexed; + } + + /** + * Build the name and guard index key. + */ + private function nameGuardIndexKey(mixed $name, mixed $guardName): string + { + // Null byte avoids collisions with names or guards containing ordinary separators. + return (string) enum_value($guardName) . "\0" . (string) enum_value($name); + } + + /** + * Index model catalog order by primary key. + * + * @return array + */ + private function indexModelOrderByKey(Collection $models): array + { + $order = []; + + foreach ($models->values() as $index => $model) { + $order[(string) $model->getKey()] = $index; + } + + return $order; + } + /** * Get the hydrated role collection for a cached permission. * From 98cbde07c8a01c77d2dcb2154d831f65682b9167 Mon Sep 17 00:00:00 2001 From: Raj Siva-Rajah <5361908+binaryfire@users.noreply.github.com> Date: Fri, 3 Jul 2026 03:30:16 +0000 Subject: [PATCH 08/14] fix(permission): harden role and permission lookups 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. --- src/permission/src/Models/Permission.php | 22 +++- src/permission/src/Models/Role.php | 38 +++++- .../Integration/PermissionRegistrarTest.php | 124 ++++++++++++++++++ .../Models/FindOrCreateRaceTest.php | 60 +++++++++ tests/Permission/Traits/TeamHasRolesTest.php | 43 ++++++ 5 files changed, 277 insertions(+), 10 deletions(-) create mode 100644 tests/Permission/Models/FindOrCreateRaceTest.php diff --git a/src/permission/src/Models/Permission.php b/src/permission/src/Models/Permission.php index b340413cb..6bd92948f 100644 --- a/src/permission/src/Models/Permission.php +++ b/src/permission/src/Models/Permission.php @@ -58,7 +58,7 @@ public static function create(array $attributes = []): PermissionContract $attributes['name'] = enum_value($attributes['name']); - $permission = static::getPermission(['name' => $attributes['name'], 'guard_name' => $attributes['guard_name']]); + $permission = static::findByParam(['name' => $attributes['name'], 'guard_name' => $attributes['guard_name']]); if ($permission) { throw PermissionAlreadyExists::create($attributes['name'], $attributes['guard_name']); @@ -143,15 +143,31 @@ public static function findOrCreate(UnitEnum|string $name, ?string $guardName = { $name = enum_value($name); $guardName ??= Guard::getDefaultName(static::class); - $permission = static::getPermission(['name' => $name, 'guard_name' => $guardName]); + $permission = static::findByParam(['name' => $name, 'guard_name' => $guardName]); if (! $permission) { - return static::query()->create(['name' => $name, 'guard_name' => $guardName]); + return static::query()->createOrFirst(['name' => $name, 'guard_name' => $guardName]); } return $permission; } + /** + * Find a permission based on an array of parameters. + * + * @return null|Permission|PermissionContract + */ + protected static function findByParam(array $params = []): ?PermissionContract + { + $query = static::query(); + + foreach ($params as $key => $value) { + $query->where($key, $value); + } + + return $query->first(); + } + /** * Get the current cached permissions. */ diff --git a/src/permission/src/Models/Role.php b/src/permission/src/Models/Role.php index 2bd56980c..d0845b8b3 100644 --- a/src/permission/src/Models/Role.php +++ b/src/permission/src/Models/Role.php @@ -124,7 +124,7 @@ public static function findByName(UnitEnum|string $name, ?string $guardName = nu $name = enum_value($name); $guardName ??= Guard::getDefaultName(static::class); - $role = static::findByParam(['name' => $name, 'guard_name' => $guardName]); + $role = static::getRole(['name' => $name, 'guard_name' => $guardName]); if (! $role) { throw RoleDoesNotExist::named($name, $guardName); @@ -142,7 +142,7 @@ public static function findById(int|string $id, ?string $guardName = null): Role { $guardName ??= Guard::getDefaultName(static::class); - $role = static::findByParam([Guard::getModelKeyName(static::class) => $id, 'guard_name' => $guardName]); + $role = static::getRole([Guard::getModelKeyName(static::class) => $id, 'guard_name' => $guardName]); if (! $role) { throw RoleDoesNotExist::withId($id, $guardName); @@ -172,7 +172,7 @@ public static function findOrCreate(UnitEnum|string $name, ?string $guardName = $attributes[$teamsKey] = getPermissionsTeamId(); } - return static::query()->create($attributes); + return static::query()->createOrFirst($attributes); } return $role; @@ -206,6 +206,26 @@ protected static function findByParam(array $params = []): ?RoleContract return $query->first(); } + /** + * Get the current cached roles. + */ + protected static function getRoles(array $params = [], bool $onlyOne = false): Collection + { + return Container::getInstance()->make(PermissionRegistrar::class) + ->getRoles($params, $onlyOne, static::class); + } + + /** + * Get the current cached first role. + * + * @return null|Role|RoleContract + */ + protected static function getRole(array $params = []): ?RoleContract + { + /** @var null|RoleContract */ + return static::getRoles($params, true)->first(); + } + /** * Determine if the role may perform the given permission. * @@ -213,16 +233,20 @@ protected static function findByParam(array $params = []): ?RoleContract */ public function hasPermissionTo(UnitEnum|int|string|PermissionContract $permission, ?string $guardName = null): bool { - if ($this->hasForbiddenPermission($permission, $guardName)) { - return false; - } - if ($this->getWildcardClass()) { + if ($this->hasForbiddenPermission($permission, $guardName)) { + return false; + } + return $this->hasWildcardPermission($permission, $guardName); } $permission = $this->filterPermission($permission, $guardName); + if ($this->hasForbiddenPermission($permission, $guardName)) { + return false; + } + if (! $this->getGuardNames()->contains($permission->guard_name)) { throw GuardDoesNotMatch::create($permission->guard_name, $guardName ? collect([$guardName]) : $this->getGuardNames()); } diff --git a/tests/Permission/Integration/PermissionRegistrarTest.php b/tests/Permission/Integration/PermissionRegistrarTest.php index 3c6241151..4f6542a82 100644 --- a/tests/Permission/Integration/PermissionRegistrarTest.php +++ b/tests/Permission/Integration/PermissionRegistrarTest.php @@ -7,6 +7,9 @@ use Hypervel\Context\CoroutineContext; use Hypervel\Permission\Contracts\Permission as PermissionContract; use Hypervel\Permission\Contracts\Role as RoleContract; +use Hypervel\Permission\Exceptions\PermissionAlreadyExists; +use Hypervel\Permission\Exceptions\PermissionDoesNotExist; +use Hypervel\Permission\Exceptions\RoleDoesNotExist; use Hypervel\Permission\Models\Permission as HypervelPermission; use Hypervel\Permission\Models\Role as HypervelRole; use Hypervel\Permission\PermissionRegistrar; @@ -117,4 +120,125 @@ public function testItCanChangeTeamId(): void $this->assertSame($teamId, $this->app->make(PermissionRegistrar::class)->getPermissionsTeamId()); } + + public function testPermissionLookupUsesGuardExactCatalogIndex(): void + { + $permissionClass = $this->app->make(PermissionContract::class); + $webPermission = $permissionClass::findByName('edit-articles', 'web'); + $apiPermission = $permissionClass::create(['name' => 'edit-articles', 'guard_name' => 'api']); + $foundApiPermission = $permissionClass::findByName('edit-articles', 'api'); + $foundApiPermissionById = $permissionClass::findById($apiPermission->getKey(), 'api'); + + $this->assertSame($webPermission->getKey(), $permissionClass::findByName('edit-articles', 'web')->getKey()); + $this->assertSame($apiPermission->getKey(), $foundApiPermission->getKey()); + $this->assertSame('api', $foundApiPermission->guard_name); + $this->assertSame($apiPermission->getKey(), $foundApiPermissionById->getKey()); + $this->assertSame('api', $foundApiPermissionById->guard_name); + } + + public function testPermissionCatalogIdLookupPreservesCatalogOrder(): void + { + $permissionClass = $this->app->make(PermissionContract::class); + $second = $permissionClass::findByName('edit-news'); + $third = $permissionClass::findByName('edit-blog'); + $registrar = $this->app->make(PermissionRegistrar::class); + + $permissions = $registrar->getPermissions([ + $second->getKeyName() => [$third->getKey(), $second->getKey()], + 'guard_name' => 'web', + ]); + + $this->assertSame( + [$second->getKey(), $third->getKey()], + $permissions->pluck($second->getKeyName())->all(), + ); + } + + public function testMissingCatalogLookupReturnsEmptyCollectionAndModelsStillThrow(): void + { + $permissionClass = $this->app->make(PermissionContract::class); + $registrar = $this->app->make(PermissionRegistrar::class); + + $this->assertTrue($registrar->getPermissions(['name' => 'missing-permission', 'guard_name' => 'web'])->isEmpty()); + + try { + $permissionClass::findByName('missing-permission'); + $this->fail('Expected missing permission exception was not thrown.'); + } catch (PermissionDoesNotExist) { + $this->assertTrue(true); + } + } + + public function testRoleLookupUsesCatalogIndexAndStillThrowsWhenMissing(): void + { + $roleClass = $this->app->make(RoleContract::class); + $role = $roleClass::findByName('testRole'); + + $this->assertTrue($role->is($roleClass::findById($role->getKey()))); + + try { + $roleClass::findByName('missing-role'); + $this->fail('Expected missing role exception was not thrown.'); + } catch (RoleDoesNotExist) { + $this->assertTrue(true); + } + } + + public function testPermissionCreateUsesDatabaseForDuplicateCheckWhenCatalogIsStale(): void + { + $permissionClass = $this->app->make(PermissionContract::class); + + $this->app->make(PermissionRegistrar::class)->getPermissions(); + + $permissionClass::query()->insert([ + 'name' => 'stale-catalog-permission', + 'guard_name' => 'web', + ]); + + $this->expectException(PermissionAlreadyExists::class); + + $permissionClass::create(['name' => 'stale-catalog-permission']); + } + + public function testPermissionFindOrCreateUsesDatabaseWhenCatalogIsStale(): void + { + $permissionClass = $this->app->make(PermissionContract::class); + + $this->app->make(PermissionRegistrar::class)->getPermissions(); + + $id = $permissionClass::query()->insertGetId([ + 'name' => 'stale-catalog-find-or-create', + 'guard_name' => 'web', + ]); + + $permission = $permissionClass::findOrCreate('stale-catalog-find-or-create'); + + $this->assertSame($id, $permission->getKey()); + $this->assertSame('stale-catalog-find-or-create', $permission->name); + $this->assertSame('web', $permission->guard_name); + } + + public function testOldCachePayloadWithoutForbiddenRolePermissionFlagIsSafe(): void + { + $registrar = $this->app->make(PermissionRegistrar::class); + + $registrar->getCacheRepository()->put($registrar->getCacheKey(), [ + 'permissions' => [], + 'roles' => [], + ], 3600); + + $registrar->clearPermissionsCollection(); + + $this->assertFalse($registrar->hasForbiddenRolePermissions()); + } + + public function testCatalogResolvedRolesExposeExpectedAttributes(): void + { + $role = $this->app->make(RoleContract::class)::findByName('testRole'); + + $this->assertSame($this->testUserRole->getKey(), $role->getKey()); + $this->assertSame('testRole', $role->name); + $this->assertSame('web', $role->guard_name); + $this->assertFalse(array_key_exists('created_at', $role->getAttributes())); + } } diff --git a/tests/Permission/Models/FindOrCreateRaceTest.php b/tests/Permission/Models/FindOrCreateRaceTest.php new file mode 100644 index 000000000..79d8274d4 --- /dev/null +++ b/tests/Permission/Models/FindOrCreateRaceTest.php @@ -0,0 +1,60 @@ +app->make(PermissionContract::class); + $insertedId = null; + + $permissionClass::creating(static function ($permission) use ($permissionClass, &$insertedId): void { + if ($permission->getAttribute('name') !== 'raced-permission' || $insertedId !== null) { + return; + } + + $insertedId = $permissionClass::query()->insertGetId([ + 'name' => 'raced-permission', + 'guard_name' => 'web', + ]); + }); + + $permission = $permissionClass::findOrCreate('raced-permission'); + + $this->assertSame($insertedId, $permission->getKey()); + $this->assertSame('raced-permission', $permission->name); + $this->assertSame('web', $permission->guard_name); + } + + public function testRoleFindOrCreateReturnsExistingRowAfterUniqueRace(): void + { + $roleClass = $this->app->make(RoleContract::class); + $insertedId = null; + + $roleClass::creating(static function ($role) use ($roleClass, &$insertedId): void { + if ($role->getAttribute('name') !== 'raced-role' || $insertedId !== null) { + return; + } + + $insertedId = $roleClass::query()->insertGetId([ + 'name' => 'raced-role', + 'guard_name' => 'web', + ]); + }); + + $role = $roleClass::findOrCreate('raced-role'); + + $this->assertSame($insertedId, $role->getKey()); + $this->assertSame('raced-role', $role->name); + $this->assertSame('web', $role->guard_name); + } +} diff --git a/tests/Permission/Traits/TeamHasRolesTest.php b/tests/Permission/Traits/TeamHasRolesTest.php index 95693fcb2..43e3b8a48 100644 --- a/tests/Permission/Traits/TeamHasRolesTest.php +++ b/tests/Permission/Traits/TeamHasRolesTest.php @@ -6,6 +6,9 @@ use Hypervel\Contracts\Foundation\Application as ApplicationContract; use Hypervel\Permission\Contracts\Role; +use Hypervel\Permission\Exceptions\RoleDoesNotExist; +use Hypervel\Permission\PermissionRegistrar; +use Hypervel\Permission\Support\Config; use Hypervel\Support\Facades\DB; use Hypervel\Tests\Permission\Fixtures\Models\User; @@ -106,6 +109,46 @@ public function testItCanAssignSameAndDifferentRolesOnSameUserDifferentTeams(): $this->assertTrue($this->testUser->hasRole($testRole4NoTeam)); } + public function testRoleLookupFindsGlobalAndCurrentTeamRolesOnly(): void + { + app(Role::class)->create(['name' => 'global-role', 'team_test_id' => null]); + app(Role::class)->create(['name' => 'team-one-role', 'team_test_id' => 1]); + app(Role::class)->create(['name' => 'team-two-role', 'team_test_id' => 2]); + + setPermissionsTeamId(1); + + $this->assertSame('global-role', app(Role::class)::findByName('global-role')->name); + $this->assertSame('team-one-role', app(Role::class)::findByName('team-one-role')->name); + + try { + app(Role::class)::findByName('team-two-role'); + $this->fail('Expected missing team role exception was not thrown.'); + } catch (RoleDoesNotExist) { + $this->assertTrue(true); + } + + setPermissionsTeamId(2); + + $this->assertSame('team-two-role', app(Role::class)::findByName('team-two-role')->name); + } + + public function testRoleLookupUsesFirstCatalogMatchForGlobalAndCurrentTeamRoleWithSameName(): void + { + app(Role::class)->create(['name' => 'shared-role', 'team_test_id' => null]); + DB::table(Config::rolesTable())->insert([ + 'name' => 'shared-role', + 'guard_name' => 'web', + 'team_test_id' => 1, + ]); + + setPermissionsTeamId(1); + + $matches = app(PermissionRegistrar::class)->getRoles(['name' => 'shared-role', 'guard_name' => 'web']); + + $this->assertCount(2, $matches); + $this->assertTrue($matches->first()->is(app(Role::class)::findByName('shared-role'))); + } + public function testItCanSyncOrRemoveRolesWithoutDetachingDifferentTeams(): void { app(Role::class)->create(['name' => 'testRole3', 'team_test_id' => 2]); From 4e672dca6c2b370954ea15e2078b8845888cc4c8 Mon Sep 17 00:00:00 2001 From: Raj Siva-Rajah <5361908+binaryfire@users.noreply.github.com> Date: Fri, 3 Jul 2026 03:30:24 +0000 Subject: [PATCH 09/14] fix(permission): enforce scoped deny semantics 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. --- src/permission/src/Traits/HasPermissions.php | 160 +++++++++++++++---- tests/Permission/CacheTest.php | 89 +++++++++++ tests/Permission/ForbiddenPermissionTest.php | 104 ++++++++++++ 3 files changed, 326 insertions(+), 27 deletions(-) diff --git a/src/permission/src/Traits/HasPermissions.php b/src/permission/src/Traits/HasPermissions.php index 073268cfa..4af37b883 100644 --- a/src/permission/src/Traits/HasPermissions.php +++ b/src/permission/src/Traits/HasPermissions.php @@ -71,6 +71,8 @@ public static function bootHasPermissions(): void ->where(Config::morphKey(), $model->getKey()) ->where('model_type', $model->getMorphClass()) ->delete(); + + Container::getInstance()->make(PermissionRegistrar::class)->forgetModelAssignmentCache($model); } if ($model instanceof Role) { @@ -86,8 +88,6 @@ public static function bootHasPermissions(): void ->where($registrar->pivotRole, $model->getKey()) ->delete(); } - - Container::getInstance()->make(PermissionRegistrar::class)->bumpModelAssignmentCacheToken(); }); static::saved(function (Model $model): void { @@ -149,7 +149,7 @@ public function permissions(): BelongsToMany } $teamsKey = Config::teamForeignKey(); - $relation->withPivot($teamsKey, 'is_forbidden'); + $relation->withPivot($teamsKey); return $relation->wherePivot($teamsKey, getPermissionsTeamId()); } @@ -400,6 +400,20 @@ public function filterPermission($permission, ?string $guardName = null): Permis */ public function hasPermissionTo($permission, ?string $guardName = null): bool { + if ($this->getWildcardClass()) { + if ($this->hasForbiddenPermission($permission, $guardName)) { + return false; + } + + if ($this->hasForbiddenPermissionViaRoles($permission, $guardName)) { + return false; + } + + return $this->hasWildcardPermission($permission, $guardName); + } + + $permission = $this->filterPermission($permission, $guardName); + if ($this->hasForbiddenPermission($permission, $guardName)) { return false; } @@ -408,12 +422,6 @@ public function hasPermissionTo($permission, ?string $guardName = null): bool return false; } - if ($this->getWildcardClass()) { - return $this->hasWildcardPermission($permission, $guardName); - } - - $permission = $this->filterPermission($permission, $guardName); - return $this->hasDirectPermission($permission) || $this->hasPermissionViaRole($permission); } @@ -545,7 +553,9 @@ public function getPermissionsViaRoles(): Collection return $permissions ->reject(fn (Model $permission): bool => isset($forbiddenPermissionKeys[$this->permissionComparisonKey($permission)])) - ->sort()->values(); + ->unique(fn (Model $permission): string => $this->permissionComparisonKey($permission)) + ->sort() + ->values(); } /** @@ -562,7 +572,7 @@ public function getAllPermissions(): Collection return $directPermissions ->merge($viaRolePermissions) ->reject(fn (Model $permission): bool => isset($forbiddenPermissionKeys[$this->permissionComparisonKey($permission)])) - ->unique(fn (Model $permission): string => (string) $permission->getKey()) + ->unique(fn (Model $permission): string => $this->permissionComparisonKey($permission)) ->sort() ->values(); } @@ -626,6 +636,7 @@ private function attachPermissions(array $permissions, bool $isForbidden): stati $model = $this; $registrar = $this->permissionRegistrar(); $pivot = $this->permissionAssignmentPivot($isForbidden); + $cacheCleared = false; if ($model->exists) { $this->upsertPermissionAssignments($permissions, $pivot); @@ -636,12 +647,16 @@ private function attachPermissions(array $permissions, bool $isForbidden): stati if ($this instanceof Role) { $this->forgetCachedPermissions(); + $cacheCleared = true; } elseif ($model->exists) { $registrar->forgetModelPermissionCache($model); + $cacheCleared = true; + } + + if (! $cacheCleared) { + $this->forgetWildcardPermissionIndex(); } - // Clear wildcard state before dispatch so synchronous listeners rebuild from current assignments. - $this->forgetWildcardPermissionIndex(); $this->dispatchPermissionAttachedEvent($permissions); return $this; @@ -787,8 +802,6 @@ protected function attachQueuedPermissionAssignments(): void } else { $registrar->forgetModelPermissionCache($this); } - - $this->forgetWildcardPermissionIndex(); } /** @@ -880,6 +893,7 @@ public function syncPermissions(...$permissions): static { $permissions = $this->collectPermissions($permissions); $model = $this; + $cacheCleared = false; if ($this->exists) { $pivot = $this->permissionAssignmentPivot(false); @@ -907,13 +921,17 @@ public function syncPermissions(...$permissions): static if ($model->exists) { if ($this instanceof Role) { $this->forgetCachedPermissions(); + $cacheCleared = true; } else { $this->permissionRegistrar()->forgetModelPermissionCache($model); + $cacheCleared = true; } } - // Clear wildcard state before dispatch so synchronous listeners rebuild from current assignments. - $this->forgetWildcardPermissionIndex(); + if (! $cacheCleared) { + $this->forgetWildcardPermissionIndex(); + } + $this->dispatchPermissionAttachedEvent($permissions); return $this; @@ -958,7 +976,6 @@ public function syncPermissionsWithForbidden(array|Collection $allowed = [], arr $allowedPivot, ); - // Clear wildcard state before dispatch so synchronous listeners rebuild from current assignments. $this->forgetWildcardPermissionIndex(); $this->dispatchPermissionAttachedEvent($permissions); @@ -986,8 +1003,6 @@ public function syncPermissionsWithForbidden(array|Collection $allowed = [], arr $this->permissionRegistrar()->forgetModelPermissionCache($model); } - // Clear wildcard state before dispatch so synchronous listeners rebuild from current assignments. - $this->forgetWildcardPermissionIndex(); $this->dispatchPermissionAttachedEvent($permissions); return $changes; @@ -1012,8 +1027,6 @@ public function revokePermissionTo($permission): static $this->dispatchPermissionDetachedEvent($storedPermission); - $this->forgetWildcardPermissionIndex(); - $this->unsetRelation('permissions'); return $this; @@ -1042,6 +1055,8 @@ protected function dispatchPermissionDetachedEvent(mixed $permission): void */ public function hasForbiddenPermission($permission, ?string $guardName = null): bool { + $guardName = $this->guardNameForPermissionMatch($permission, $guardName); + return $this->getCachedDirectPermissions() ->contains( fn (Model $storedPermission): bool => $this->pivotIsForbidden($storedPermission) @@ -1060,10 +1075,31 @@ public function hasForbiddenPermissionViaRoles($permission, ?string $guardName = return false; } - return $this->getPermissionsViaRolesWithPivots() + $roles = $this->getCachedRoles(); + + if ($roles->isEmpty()) { + return false; + } + + $registrar = $this->permissionRegistrar(); + + if (! $registrar->hasForbiddenRolePermissions()) { + return false; + } + + $guardName = $this->guardNameForPermissionMatch($permission, $guardName); + $storedPermission = $this->permissionForMatch($permission, $guardName); + + if (! $storedPermission instanceof Model) { + return false; + } + + $roleIds = array_flip($roles->map(fn (Model $role): string => (string) $role->getKey())->all()); + + return $this->relationCollection($storedPermission, 'roles') ->contains( - fn (Model $storedPermission): bool => $this->pivotIsForbidden($storedPermission) - && $this->storedPermissionMatches($storedPermission, $permission, $guardName) + fn (Model $role): bool => isset($roleIds[(string) $role->getKey()]) + && $this->pivotIsForbidden($role) ); } @@ -1076,24 +1112,74 @@ protected function getPermissionsViaRolesWithPivots(): Collection return collect(); } + if (! $this->exists) { + return $this->loadPermissionsViaRolesWithPivots(); + } + + return $this->permissionRegistrar()->rememberModelViaRolePermissions( + $this, + fn (): Collection => $this->loadPermissionsViaRolesWithPivots(), + ); + } + + /** + * Load permissions granted through the model's roles with role-permission pivot data. + */ + protected function loadPermissionsViaRolesWithPivots(): Collection + { + if ($this instanceof Role || $this instanceof Permission) { + return collect(); + } + $roles = $this->getCachedRoles(); if ($roles->isEmpty()) { return collect(); } - $roleKey = Guard::getModelKeyName($this->getRoleClass()); $roleIds = array_flip($roles->map(fn (Model $role): string => (string) $role->getKey())->all()); return $this->permissionRegistrar() ->getPermissions([], false, $this->getPermissionClass()) ->flatMap( fn (Model $permission): Collection => $this->relationCollection($permission, 'roles') - ->filter(fn (Model $role): bool => isset($roleIds[(string) $role->getAttribute($roleKey)])) + ->filter(fn (Model $role): bool => isset($roleIds[(string) $role->getKey()])) ->map(fn (Model $role): Model => $this->permissionWithRolePivot($permission, $role)) ); } + /** + * Resolve a permission for matching without throwing. + */ + protected function permissionForMatch(mixed $permission, string $guardName): ?Model + { + $permissionKey = Guard::getModelKeyName($this->getPermissionClass()); + + if ($permission instanceof Permission) { + if (! $permission instanceof Model || $permission->guard_name !== $guardName) { + return null; + } + + return $this->permissionRegistrar() + ->getPermissions([$permissionKey => $permission->getKey(), 'guard_name' => $guardName], true, $this->getPermissionClass()) + ->first(); + } + + $permission = enum_value($permission); + + if (! is_string($permission) && ! is_int($permission)) { + return null; + } + + $params = is_int($permission) || PermissionRegistrar::isUid($permission) + ? [$permissionKey => $permission, 'guard_name' => $guardName] + : ['name' => $permission, 'guard_name' => $guardName]; + + return $this->permissionRegistrar() + ->getPermissions($params, true, $this->getPermissionClass()) + ->first(); + } + /** * Build lookup keys for forbidden permissions. */ @@ -1182,6 +1268,26 @@ protected function storedPermissionMatches(Model $storedPermission, mixed $permi && $storedPermission->getAttribute('name') === $permission; } + /** + * Resolve the guard to use when matching stored permissions. + */ + protected function guardNameForPermissionMatch(mixed $permission, ?string $guardName = null): string + { + if ($guardName !== null) { + return $guardName; + } + + if ($permission instanceof Permission) { + $permissionGuard = $permission->guard_name ?? null; + + if (is_string($permissionGuard) && $permissionGuard !== '') { + return $permissionGuard; + } + } + + return $this->getDefaultGuardName(); + } + /** * Get the permission names. */ diff --git a/tests/Permission/CacheTest.php b/tests/Permission/CacheTest.php index 25035210c..8f34a0fd2 100644 --- a/tests/Permission/CacheTest.php +++ b/tests/Permission/CacheTest.php @@ -6,8 +6,10 @@ use Hypervel\Context\CoroutineContext; use Hypervel\Permission\Contracts\Permission as PermissionContract; +use Hypervel\Permission\Contracts\Role as RoleContract; use Hypervel\Permission\PermissionRegistrar; use Hypervel\Permission\Support\Config; +use Hypervel\Tests\Permission\Fixtures\Models\User; use function Hypervel\Coroutine\parallel; @@ -78,6 +80,29 @@ public function testRoleAssignmentMutationsInvalidateWarmModelRoleCache(): void $this->assertTrue($this->testUser->hasRole('testRole2')); } + public function testRoleAssignmentMutationsInvalidateWarmViaRolePermissionMemo(): void + { + $secondRole = $this->app->make(RoleContract::class)::findByName('testRole2'); + + $this->testUserRole->givePermissionTo('edit-articles'); + $secondRole->givePermissionTo('edit-news'); + $this->testUser->assignRole($this->testUserRole); + + $this->assertSame(['edit-articles'], $this->testUser->getPermissionsViaRoles()->pluck('name')->all()); + + $this->testUser->syncRoles($secondRole); + + $this->assertSame(['edit-news'], $this->testUser->getPermissionsViaRoles()->pluck('name')->all()); + } + + public function testUnsavedModelsDoNotUseViaRolePermissionMemo(): void + { + $user = new User(['email' => 'unsaved@user.com']); + + $this->assertSame([], $user->getPermissionsViaRoles()->all()); + $this->assertSame([], CoroutineContext::get(PermissionRegistrar::MODEL_VIA_ROLE_PERMISSIONS_CONTEXT_KEY, [])); + } + public function testDirectPermissionMutationsInvalidateWarmModelPermissionCache(): void { $this->assertFalse($this->testUser->hasPermissionTo('edit-articles')); @@ -117,6 +142,17 @@ public function testRolePermissionMutationsInvalidateWarmGlobalPermissionCatalog $this->assertFalse($this->testUser->hasPermissionTo('edit-articles')); } + public function testRolePermissionMutationsInvalidateWarmViaRolePermissionMemo(): void + { + $this->testUser->assignRole('testRole'); + + $this->assertSame([], $this->testUser->getAllPermissions()->pluck('name')->all()); + + $this->testUserRole->givePermissionTo('edit-articles'); + + $this->assertSame(['edit-articles'], $this->testUser->getAllPermissions()->pluck('name')->all()); + } + public function testRoleForbiddenPermissionMutationsInvalidateWarmGlobalPermissionCatalog(): void { $this->testUser->assignRole('testRole'); @@ -251,6 +287,34 @@ public function testCustomCacheKeyResolverPreventsCrossTenantBleedForRoleGranted $this->assertFalse($this->testUser->hasPermissionTo('edit-articles')); } + public function testCustomCacheKeyResolverPreventsCrossTenantBleedForViaRolePermissionMemo(): void + { + $tenant = 'tenant-a'; + PermissionRegistrar::resolveCacheKeyUsing(function () use (&$tenant): string { + return $tenant; + }); + + $this->testUser->assignRole('testRole'); + $this->testUserRole->givePermissionTo('edit-articles'); + + $this->assertSame(['edit-articles'], $this->testUser->getPermissionsViaRoles()->pluck('name')->all()); + + $connection = $this->testUserRole->getConnection(); + $connection->table(Config::modelHasRolesTable())->delete(); + $connection->table(Config::roleHasPermissionsTable())->delete(); + + $this->assertSame(['edit-articles'], $this->testUser->getPermissionsViaRoles()->pluck('name')->all()); + + $tenant = 'tenant-b'; + + $this->assertSame([], $this->testUser->getPermissionsViaRoles()->pluck('name')->all()); + + $keys = array_keys(CoroutineContext::get(PermissionRegistrar::MODEL_VIA_ROLE_PERMISSIONS_CONTEXT_KEY, [])); + + $this->assertNotEmpty(array_filter($keys, fn (string $key): bool => str_starts_with($key, 'tenant-a:'))); + $this->assertNotEmpty(array_filter($keys, fn (string $key): bool => str_starts_with($key, 'tenant-b:'))); + } + public function testCustomCacheKeyResolverPreventsCrossTenantBleedForDirectPermissions(): void { $tenant = 'tenant-a'; @@ -333,25 +397,50 @@ public function testFlushStateClearsCustomCacheKeyResolver(): void ); } + public function testDeletingPlainModelDoesNotBumpGlobalAssignmentCacheToken(): void + { + $anotherUser = User::create(['email' => 'another@user.com']); + $this->testUserRole->givePermissionTo('edit-articles'); + $this->testUser->assignRole('testRole'); + $anotherUser->assignRole('testRole'); + $registrar = $this->app->make(PermissionRegistrar::class); + + $this->assertTrue($this->testUser->hasPermissionTo('edit-articles')); + $this->assertTrue($anotherUser->hasPermissionTo('edit-articles')); + + $token = $registrar->modelAssignmentCacheToken(); + + $this->testUser->delete(); + + $this->assertSame($token, $registrar->modelAssignmentCacheToken()); + $this->assertTrue($anotherUser->hasPermissionTo('edit-articles')); + } + public function testDeletingRoleCleansRolePermissionPivotWithoutForeignKeyCascades(): void { $this->testUserRole->givePermissionTo('edit-articles'); + $registrar = $this->app->make(PermissionRegistrar::class); + $token = $registrar->modelAssignmentCacheToken(); $this->assertSame(1, $this->testUserRole->getConnection()->table(Config::roleHasPermissionsTable())->count()); $this->testUserRole->delete(); $this->assertSame(0, $this->testUserRole->getConnection()->table(Config::roleHasPermissionsTable())->count()); + $this->assertNotSame($token, $registrar->modelAssignmentCacheToken()); } public function testDeletingPermissionCleansRolePermissionPivotWithoutForeignKeyCascades(): void { $this->testUserRole->givePermissionTo('edit-articles'); + $registrar = $this->app->make(PermissionRegistrar::class); + $token = $registrar->modelAssignmentCacheToken(); $this->assertSame(1, $this->testUserRole->getConnection()->table(Config::roleHasPermissionsTable())->count()); $this->testUserPermission->delete(); $this->assertSame(0, $this->testUserRole->getConnection()->table(Config::roleHasPermissionsTable())->count()); + $this->assertNotSame($token, $registrar->modelAssignmentCacheToken()); } } diff --git a/tests/Permission/ForbiddenPermissionTest.php b/tests/Permission/ForbiddenPermissionTest.php index f0c4826b9..5008e004a 100644 --- a/tests/Permission/ForbiddenPermissionTest.php +++ b/tests/Permission/ForbiddenPermissionTest.php @@ -7,6 +7,7 @@ use Hypervel\Database\Eloquent\Relations\Pivot; use Hypervel\Permission\Contracts\Permission as PermissionContract; use Hypervel\Permission\Contracts\Role as RoleContract; +use Hypervel\Permission\Exceptions\PermissionDoesNotExist; use Hypervel\Permission\PermissionRegistrar; use Hypervel\Permission\Support\Config; use Hypervel\Tests\Permission\Fixtures\Models\Permission; @@ -65,6 +66,21 @@ public function testDirectForbiddenPermissionOverridesRolePermission(): void $this->assertFalse($this->testUser->getAllPermissions()->contains('name', 'edit-articles')); } + public function testDirectForbiddenPermissionIsGuardExact(): void + { + $permissionClass = $this->app->make(PermissionContract::class); + $webPermission = $permissionClass::findByName('edit-articles', 'web'); + $apiPermission = $permissionClass::create(['name' => 'edit-articles', 'guard_name' => 'api']); + + $this->testUser->givePermissionTo($webPermission); + $this->testUser->giveForbiddenTo($apiPermission); + + $this->assertTrue($this->testUser->hasPermissionTo('edit-articles')); + $this->assertFalse($this->testUser->hasPermissionTo('edit-articles', 'api')); + $this->assertFalse($this->testUser->hasPermissionTo($apiPermission)); + $this->assertTrue($this->testUser->hasPermissionTo($webPermission)); + } + public function testRoleForbiddenPermissionOverridesDirectPermission(): void { $role = $this->app->make(RoleContract::class)::create(['name' => 'restricted']); @@ -118,6 +134,67 @@ public function testRoleForbiddenPermissionOverridesAllowedRolePermission(): voi $this->assertFalse($this->testUser->getAllPermissions()->contains('name', 'edit-articles')); } + public function testRoleForbiddenPermissionIsGuardExact(): void + { + $permissionClass = $this->app->make(PermissionContract::class); + $roleClass = $this->app->make(RoleContract::class); + $webPermission = $permissionClass::findByName('edit-articles', 'web'); + $apiPermission = $permissionClass::create(['name' => 'edit-articles', 'guard_name' => 'api']); + $webRole = $roleClass::create(['name' => 'web-editor']); + $apiRole = $roleClass::create(['name' => 'api-blocked', 'guard_name' => 'api']); + + $webRole->givePermissionTo($webPermission); + $apiRole->giveForbiddenTo($apiPermission); + $this->testUser->assignRole($webRole, $apiRole); + $webPermission = $permissionClass::findByName('edit-articles', 'web'); + + $this->assertTrue($this->testUser->hasPermissionTo('edit-articles')); + $this->assertFalse($this->testUser->hasPermissionTo('edit-articles', 'api')); + $this->assertFalse($this->testUser->hasPermissionTo($apiPermission)); + $this->assertTrue($this->testUser->hasPermissionTo($webPermission)); + } + + public function testRoleForbiddenPermissionForDifferentPermissionDoesNotDenyRequestedPermission(): void + { + $allowedRole = $this->app->make(RoleContract::class)::create(['name' => 'allowed-editor']); + $forbiddenRole = $this->app->make(RoleContract::class)::create(['name' => 'blocked-news']); + + $allowedRole->givePermissionTo('edit-articles'); + $forbiddenRole->giveForbiddenTo('edit-news'); + $this->testUser->assignRole($allowedRole, $forbiddenRole); + + $this->assertTrue($this->testUser->hasPermissionTo('edit-articles')); + $this->assertFalse($this->testUser->hasPermissionTo('edit-news')); + } + + public function testMissingPermissionStillThrowsOrChecksFalseWithRoleForbiddenEdges(): void + { + $this->testUserRole->giveForbiddenTo('edit-articles'); + $this->testUser->assignRole($this->testUserRole); + + $this->assertFalse($this->testUser->checkPermissionTo('missing-permission')); + + $this->expectException(PermissionDoesNotExist::class); + + $this->testUser->hasPermissionTo('missing-permission'); + } + + public function testRoleModelForbiddenPermissionIsMatchedByConcretePermissionGuard(): void + { + $permission = $this->app->make(PermissionContract::class)::create([ + 'name' => 'api-edit-articles', + 'guard_name' => 'api', + ]); + $role = $this->app->make(RoleContract::class)::create([ + 'name' => 'api-editor', + 'guard_name' => 'api', + ]); + + $role->giveForbiddenTo($permission); + + $this->assertFalse($role->hasPermissionTo($permission)); + } + public function testForbiddenPermissionWinsWhenAllowedAndForbiddenAreSyncedTogether(): void { $changes = $this->testUser->syncPermissionsWithForbidden( @@ -301,6 +378,33 @@ public function testRoleForbiddenPermissionsAreExcludedFromRolePermissionResults $this->assertFalse($this->testUser->hasPermissionTo('delete-articles')); } + public function testDuplicateRoleGrantedPermissionsAreReturnedOnce(): void + { + $firstRole = $this->app->make(RoleContract::class)::create(['name' => 'first-editor']); + $secondRole = $this->app->make(RoleContract::class)::create(['name' => 'second-editor']); + + $firstRole->givePermissionTo('edit-articles'); + $secondRole->givePermissionTo('edit-articles'); + $this->testUser->assignRole($firstRole, $secondRole); + + $this->assertSame( + ['edit-articles'], + $this->testUser->getPermissionsViaRoles()->pluck('name')->values()->all(), + ); + } + + public function testForbiddenDuplicateRolePermissionIsExcluded(): void + { + $allowedRole = $this->app->make(RoleContract::class)::create(['name' => 'duplicate-allowed']); + $forbiddenRole = $this->app->make(RoleContract::class)::create(['name' => 'duplicate-forbidden']); + + $allowedRole->givePermissionTo('edit-articles'); + $forbiddenRole->giveForbiddenTo('edit-articles'); + $this->testUser->assignRole($allowedRole, $forbiddenRole); + + $this->assertSame([], $this->testUser->getPermissionsViaRoles()->pluck('name')->values()->all()); + } + public function testRoleForbiddenSyncAffectsAllUsersWithRoleAfterCachesAreWarm(): void { $role = $this->app->make(RoleContract::class)::create(['name' => 'publisher']); From dce4912a27900a2e91e1e8afe6bc9b2572bf735c Mon Sep 17 00:00:00 2001 From: Raj Siva-Rajah <5361908+binaryfire@users.noreply.github.com> Date: Fri, 3 Jul 2026 03:30:30 +0000 Subject: [PATCH 10/14] docs(permission): document final cache and deny behavior 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. --- src/boost/docs/permission.md | 20 +++----------------- src/permission/README.md | 2 ++ 2 files changed, 5 insertions(+), 17 deletions(-) diff --git a/src/boost/docs/permission.md b/src/boost/docs/permission.md index b1e14576d..61572b814 100644 --- a/src/boost/docs/permission.md +++ b/src/boost/docs/permission.md @@ -6,7 +6,6 @@ - [Running Migrations](#running-migrations) - [Configuration](#configuration) - [Models](#models) - - [Database Connection](#database-connection) - [Table and Column Names](#table-and-column-names) - [Cache](#cache) - [Model Setup](#model-setup) @@ -128,21 +127,6 @@ You may customize the models used for roles and permissions: Custom role models must implement the `Hypervel\Permission\Contracts\Role` contract. Custom permission models must implement the `Hypervel\Permission\Contracts\Permission` contract. The easiest way to satisfy these contracts is to extend the package's base models. - -### Database Connection - -You may store the permission tables on a specific database connection: - -```php -'storage' => [ - 'database' => [ - 'connection' => env('DB_CONNECTION', 'mysql'), - ], -], -``` - -The published migration reads this value when choosing its migration connection. - ### Table and Column Names @@ -1091,7 +1075,7 @@ public function boot(): void } ``` -The resolver adds a context segment to the global permission catalog, model assignment caches, assignment-cache token, and wildcard permission indexes. Since the resolver is called during each cache-key build, it can safely read request-specific coroutine context. Teams still scope inside this context, so a multi-tenant app can have independent teams for each tenant. +The resolver adds a context segment to the global permission catalog, model assignment caches, assignment-cache token, via-role permission memo, and wildcard permission indexes. Since the resolver is called during each cache-key build, it can safely read request-specific coroutine context. Teams still scope inside this context, so a multi-tenant app can have independent teams for each tenant. **Why a static callback, not a config closure?** Config files are evaluated once at boot in Swoole. A closure calling `tenantId()` in config would capture the boot-time tenant (likely null), not the per-request tenant. The static resolver callback runs fresh when permission cache keys are built, reading the current coroutine's context. @@ -1227,5 +1211,7 @@ $exception->getRequiredPermissions(); ## Differences From Spatie Laravel Permission - Hypervel adds forbidden permissions. A forbidden permission explicitly denies an ability and wins over direct or role-granted allows. The deny flag is stored as the effect on the assignment row, so assigning allow or deny for the same model or role and permission updates the existing edge. +- `getDirectPermissions()`, `getPermissionsViaRoles()`, `getAllPermissions()`, and `getPermissionNames()` return effective allowed permissions. Explicit denies are exposed through `hasForbiddenPermission()` and `hasForbiddenPermissionViaRoles()`. - Hypervel accepts pure unit enums anywhere enum names are valid role or permission inputs. Backed enums use their values; unit enums use their case names. - Hypervel's cache config uses `expiration_seconds` and separate named cache keys so role, model-role, model-permission, and assignment-token caches can be invalidated independently. +- Undefined `permission.cache.store` values fail fast through Hypervel's cache manager instead of silently falling back to an array store. diff --git a/src/permission/README.md b/src/permission/README.md index 7fe473dab..348dae724 100644 --- a/src/permission/README.md +++ b/src/permission/README.md @@ -41,8 +41,10 @@ class User extends Model ## Differences From Spatie Laravel Permission - Hypervel adds forbidden permissions. A forbidden permission explicitly denies an ability and wins over direct or role-granted allows. The deny flag is stored as the effect on the assignment row, so assigning allow or deny for the same model or role and permission updates the existing edge. Use `syncPermissionsWithForbidden()` to replace allowed and forbidden assignments together. +- `getDirectPermissions()`, `getPermissionsViaRoles()`, `getAllPermissions()`, and `getPermissionNames()` return effective allowed permissions. Explicit denies are exposed through `hasForbiddenPermission()` and `hasForbiddenPermissionViaRoles()`. - Hypervel accepts pure unit enums anywhere enum names are valid role or permission inputs. Backed enums use their values; unit enums use their case names. - Hypervel's cache config uses `expiration_seconds` and separate named cache keys so role, model-role, model-permission, and assignment-token caches can be invalidated independently. - Apps where permission data depends on request context, such as multi-tenant apps with tenant-scoped permission tables, may register a runtime cache key resolver with `PermissionRegistrar::resolveCacheKeyUsing(...)` so cached permission catalogs and assignments are isolated per context. +- Undefined `permission.cache.store` values fail fast through Hypervel's cache manager instead of silently falling back to an array store. Full usage docs are available in `src/boost/docs/permission.md`. From 79745d08e59bdd164239fae17006e9ad280d1001 Mon Sep 17 00:00:00 2001 From: Raj Siva-Rajah <5361908+binaryfire@users.noreply.github.com> Date: Fri, 3 Jul 2026 04:46:17 +0000 Subject: [PATCH 11/14] fix(permission): make direct creation race-safe 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(). --- src/permission/src/Models/Permission.php | 12 +++++- src/permission/src/Models/Role.php | 12 +++++- .../Models/FindOrCreateRaceTest.php | 42 +++++++++++++++++++ 3 files changed, 64 insertions(+), 2 deletions(-) diff --git a/src/permission/src/Models/Permission.php b/src/permission/src/Models/Permission.php index 6bd92948f..7fbb86ede 100644 --- a/src/permission/src/Models/Permission.php +++ b/src/permission/src/Models/Permission.php @@ -9,6 +9,7 @@ use Hypervel\Database\Eloquent\Collection; use Hypervel\Database\Eloquent\Model; use Hypervel\Database\Eloquent\Relations\BelongsToMany; +use Hypervel\Database\UniqueConstraintViolationException; use Hypervel\Permission\Contracts\Permission as PermissionContract; use Hypervel\Permission\Exceptions\PermissionAlreadyExists; use Hypervel\Permission\Exceptions\PermissionDoesNotExist; @@ -64,7 +65,16 @@ public static function create(array $attributes = []): PermissionContract throw PermissionAlreadyExists::create($attributes['name'], $attributes['guard_name']); } - return static::query()->create($attributes); + $query = static::query(); + + try { + /** @var PermissionContract $permission */ + $permission = $query->withSavepointIfNeeded(fn () => $query->create($attributes)); + + return $permission; + } catch (UniqueConstraintViolationException) { + throw PermissionAlreadyExists::create($attributes['name'], $attributes['guard_name']); + } } /** diff --git a/src/permission/src/Models/Role.php b/src/permission/src/Models/Role.php index d0845b8b3..fab46426f 100644 --- a/src/permission/src/Models/Role.php +++ b/src/permission/src/Models/Role.php @@ -9,6 +9,7 @@ use Hypervel\Database\Eloquent\Collection; use Hypervel\Database\Eloquent\Model; use Hypervel\Database\Eloquent\Relations\BelongsToMany; +use Hypervel\Database\UniqueConstraintViolationException; use Hypervel\Permission\Contracts\Permission as PermissionContract; use Hypervel\Permission\Contracts\Role as RoleContract; use Hypervel\Permission\Exceptions\GuardDoesNotMatch; @@ -80,7 +81,16 @@ public static function create(array $attributes = []): RoleContract throw RoleAlreadyExists::create($attributes['name'], $attributes['guard_name']); } - return static::query()->create($attributes); + $query = static::query(); + + try { + /** @var RoleContract $role */ + $role = $query->withSavepointIfNeeded(fn () => $query->create($attributes)); + + return $role; + } catch (UniqueConstraintViolationException) { + throw RoleAlreadyExists::create($attributes['name'], $attributes['guard_name']); + } } /** diff --git a/tests/Permission/Models/FindOrCreateRaceTest.php b/tests/Permission/Models/FindOrCreateRaceTest.php index 79d8274d4..096a54f1e 100644 --- a/tests/Permission/Models/FindOrCreateRaceTest.php +++ b/tests/Permission/Models/FindOrCreateRaceTest.php @@ -6,6 +6,8 @@ use Hypervel\Permission\Contracts\Permission as PermissionContract; use Hypervel\Permission\Contracts\Role as RoleContract; +use Hypervel\Permission\Exceptions\PermissionAlreadyExists; +use Hypervel\Permission\Exceptions\RoleAlreadyExists; use Hypervel\Tests\Permission\TestCase; class FindOrCreateRaceTest extends TestCase @@ -57,4 +59,44 @@ public function testRoleFindOrCreateReturnsExistingRowAfterUniqueRace(): void $this->assertSame('raced-role', $role->name); $this->assertSame('web', $role->guard_name); } + + public function testPermissionCreateThrowsPackageExceptionAfterUniqueRace(): void + { + $permissionClass = $this->app->make(PermissionContract::class); + + $permissionClass::creating(static function ($permission) use ($permissionClass): void { + if ($permission->getAttribute('name') !== 'raced-permission-create') { + return; + } + + $permissionClass::query()->insert([ + 'name' => 'raced-permission-create', + 'guard_name' => 'web', + ]); + }); + + $this->expectException(PermissionAlreadyExists::class); + + $permissionClass::create(['name' => 'raced-permission-create']); + } + + public function testRoleCreateThrowsPackageExceptionAfterUniqueRace(): void + { + $roleClass = $this->app->make(RoleContract::class); + + $roleClass::creating(static function ($role) use ($roleClass): void { + if ($role->getAttribute('name') !== 'raced-role-create') { + return; + } + + $roleClass::query()->insert([ + 'name' => 'raced-role-create', + 'guard_name' => 'web', + ]); + }); + + $this->expectException(RoleAlreadyExists::class); + + $roleClass::create(['name' => 'raced-role-create']); + } } From ff5260f157ed00f317c924fe16eb9cde507192c7 Mon Sep 17 00:00:00 2001 From: Raj Siva-Rajah <5361908+binaryfire@users.noreply.github.com> Date: Fri, 3 Jul 2026 04:46:24 +0000 Subject: [PATCH 12/14] test(permission): cover team-aware role find or 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. --- tests/Permission/Traits/TeamHasRolesTest.php | 34 ++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/tests/Permission/Traits/TeamHasRolesTest.php b/tests/Permission/Traits/TeamHasRolesTest.php index 43e3b8a48..1ac3b723f 100644 --- a/tests/Permission/Traits/TeamHasRolesTest.php +++ b/tests/Permission/Traits/TeamHasRolesTest.php @@ -149,6 +149,40 @@ public function testRoleLookupUsesFirstCatalogMatchForGlobalAndCurrentTeamRoleWi $this->assertTrue($matches->first()->is(app(Role::class)::findByName('shared-role'))); } + public function testRoleFindOrCreateCreatesCurrentTeamRoleWhenOnlyAnotherTeamHasSameName(): void + { + app(Role::class)->create(['name' => 'team-find-or-create', 'team_test_id' => 2]); + + setPermissionsTeamId(1); + + $role = app(Role::class)::findOrCreate('team-find-or-create'); + + $this->assertSame(1, $role->team_test_id); + $this->assertDatabaseHas(Config::rolesTable(), [ + 'name' => 'team-find-or-create', + 'guard_name' => 'web', + 'team_test_id' => 1, + ]); + $this->assertSame(2, app(Role::class)->where('name', 'team-find-or-create')->count()); + } + + public function testRoleFindOrCreateReturnsGlobalRoleInCurrentTeamScope(): void + { + $globalRoleId = DB::table(Config::rolesTable())->insertGetId([ + 'name' => 'global-find-or-create', + 'guard_name' => 'web', + 'team_test_id' => null, + ]); + + setPermissionsTeamId(1); + + $role = app(Role::class)::findOrCreate('global-find-or-create'); + + $this->assertSame($globalRoleId, $role->getKey()); + $this->assertNull($role->team_test_id); + $this->assertSame(1, app(Role::class)->where('name', 'global-find-or-create')->count()); + } + public function testItCanSyncOrRemoveRolesWithoutDetachingDifferentTeams(): void { app(Role::class)->create(['name' => 'testRole3', 'team_test_id' => 2]); From f415fbc59328a0f6747e133ab4ed07c42f807fc0 Mon Sep 17 00:00:00 2001 From: Raj Siva-Rajah <5361908+binaryfire@users.noreply.github.com> Date: Fri, 3 Jul 2026 04:46:31 +0000 Subject: [PATCH 13/14] test(permission): add package database integration convention 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. --- .github/workflows/databases.yml | 14 ++--- .../PermissionCreateTransactionTest.php | 60 +++++++++++++++++++ 2 files changed, 67 insertions(+), 7 deletions(-) create mode 100644 tests/Integration/Permission/Database/Postgres/PermissionCreateTransactionTest.php diff --git a/.github/workflows/databases.yml b/.github/workflows/databases.yml index 1bb47efe6..acdbf280e 100644 --- a/.github/workflows/databases.yml +++ b/.github/workflows/databases.yml @@ -62,7 +62,7 @@ jobs: DB_DATABASE: testing DB_USERNAME: root DB_PASSWORD: password - run: vendor/bin/phpunit tests/Integration/Database + run: vendor/bin/phpunit tests/Integration/Database $(find tests/Integration -path 'tests/Integration/*/Database/MySql' -type d) mysql_9: runs-on: ubuntu-latest @@ -118,7 +118,7 @@ jobs: DB_DATABASE: testing DB_USERNAME: root DB_PASSWORD: password - run: vendor/bin/phpunit tests/Integration/Database + run: vendor/bin/phpunit tests/Integration/Database $(find tests/Integration -path 'tests/Integration/*/Database/MySql' -type d) mariadb_10: runs-on: ubuntu-latest @@ -174,7 +174,7 @@ jobs: DB_DATABASE: testing DB_USERNAME: root DB_PASSWORD: password - run: vendor/bin/phpunit tests/Integration/Database + run: vendor/bin/phpunit tests/Integration/Database $(find tests/Integration -path 'tests/Integration/*/Database/MariaDb' -type d) mariadb_11: runs-on: ubuntu-latest @@ -230,7 +230,7 @@ jobs: DB_DATABASE: testing DB_USERNAME: root DB_PASSWORD: password - run: vendor/bin/phpunit tests/Integration/Database + run: vendor/bin/phpunit tests/Integration/Database $(find tests/Integration -path 'tests/Integration/*/Database/MariaDb' -type d) pgsql_17: runs-on: ubuntu-latest @@ -286,7 +286,7 @@ jobs: DB_DATABASE: testing DB_USERNAME: postgres DB_PASSWORD: password - run: vendor/bin/phpunit tests/Integration/Database + run: vendor/bin/phpunit tests/Integration/Database $(find tests/Integration -path 'tests/Integration/*/Database/Postgres' -type d) pgsql_18: runs-on: ubuntu-latest @@ -342,7 +342,7 @@ jobs: DB_DATABASE: testing DB_USERNAME: postgres DB_PASSWORD: password - run: vendor/bin/phpunit tests/Integration/Database + run: vendor/bin/phpunit tests/Integration/Database $(find tests/Integration -path 'tests/Integration/*/Database/Postgres' -type d) sqlite: runs-on: ubuntu-latest @@ -382,4 +382,4 @@ jobs: DB_DATABASE: /tmp/testing.sqlite run: | touch /tmp/testing.sqlite - vendor/bin/phpunit tests/Integration/Database + vendor/bin/phpunit tests/Integration/Database $(find tests/Integration -path 'tests/Integration/*/Database/Sqlite' -type d) diff --git a/tests/Integration/Permission/Database/Postgres/PermissionCreateTransactionTest.php b/tests/Integration/Permission/Database/Postgres/PermissionCreateTransactionTest.php new file mode 100644 index 000000000..bc605aa93 --- /dev/null +++ b/tests/Integration/Permission/Database/Postgres/PermissionCreateTransactionTest.php @@ -0,0 +1,60 @@ +make('config')->set('database.default', getenv('DB_CONNECTION') ?: 'testing'); + } + + public function testCreateRaceExceptionDoesNotPoisonPostgresTransaction(): void + { + $permissionClass = $this->app->make(PermissionContract::class); + + $permissionClass::creating(static function ($permission) use ($permissionClass): void { + if ($permission->getAttribute('name') !== 'postgres-raced-permission') { + return; + } + + $permissionClass::query()->insert([ + 'name' => 'postgres-raced-permission', + 'guard_name' => 'web', + ]); + }); + + DB::transaction(function () use ($permissionClass): void { + try { + $permissionClass::create(['name' => 'postgres-raced-permission']); + $this->fail('Expected duplicate permission exception was not thrown.'); + } catch (PermissionAlreadyExists) { + $this->assertTrue(true); + } + + $permission = $permissionClass::create(['name' => 'postgres-transaction-still-usable']); + + $this->assertSame('postgres-transaction-still-usable', $permission->name); + }); + + $this->assertDatabaseHas(Config::permissionsTable(), [ + 'name' => 'postgres-transaction-still-usable', + 'guard_name' => 'web', + ]); + } +} From 6cab5cea346c058ccc38a0bce0590b0e82065154 Mon Sep 17 00:00:00 2001 From: Raj Siva-Rajah <5361908+binaryfire@users.noreply.github.com> Date: Fri, 3 Jul 2026 05:48:11 +0000 Subject: [PATCH 14/14] fix(permission): restore soft-deleted records on find or create 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. --- src/boost/docs/permission.md | 6 ++++++ src/permission/README.md | 1 + src/permission/src/Models/Permission.php | 10 +++++++++- src/permission/src/Models/Role.php | 9 ++++++++- .../HasPermissionsWithCustomModelsTest.php | 19 +++++++++++++++++++ .../Traits/HasRolesWithCustomModelsTest.php | 19 +++++++++++++++++++ 6 files changed, 62 insertions(+), 2 deletions(-) diff --git a/src/boost/docs/permission.md b/src/boost/docs/permission.md index 61572b814..72477272f 100644 --- a/src/boost/docs/permission.md +++ b/src/boost/docs/permission.md @@ -1026,6 +1026,12 @@ After creating custom models, update the permission configuration: If you replace the package models instead of extending them, your role model must implement `Hypervel\Permission\Contracts\Role` and your permission model must implement `Hypervel\Permission\Contracts\Permission`. +The package's default role and permission models do not use soft deletes, and soft deletes are not recommended for permission models. Roles and permissions are access-control records; deleting one should normally remove its assignments, not leave them waiting to become active again later. + +If you use a custom role or permission model that uses `SoftDeletes`, soft-deleting a role or permission hides it from normal permission checks, but its assignment rows remain in the database. If the role or permission is restored, those assignments become active again. For roles, previous user-role and role-permission assignments become active again. For permissions, previous direct model-permission and role-permission assignments become active again. + +Use hard deletes for roles and permissions when assignments should be removed permanently. + ## UUID and ULID Keys diff --git a/src/permission/README.md b/src/permission/README.md index 348dae724..4b6a873c3 100644 --- a/src/permission/README.md +++ b/src/permission/README.md @@ -46,5 +46,6 @@ class User extends Model - Hypervel's cache config uses `expiration_seconds` and separate named cache keys so role, model-role, model-permission, and assignment-token caches can be invalidated independently. - Apps where permission data depends on request context, such as multi-tenant apps with tenant-scoped permission tables, may register a runtime cache key resolver with `PermissionRegistrar::resolveCacheKeyUsing(...)` so cached permission catalogs and assignments are isolated per context. - Undefined `permission.cache.store` values fail fast through Hypervel's cache manager instead of silently falling back to an array store. +- The default role and permission models do not use soft deletes. Soft deletes are not recommended for permission models; restoring a custom soft-deletable role or permission reactivates its existing assignments. Full usage docs are available in `src/boost/docs/permission.md`. diff --git a/src/permission/src/Models/Permission.php b/src/permission/src/Models/Permission.php index 7fbb86ede..d6f80837f 100644 --- a/src/permission/src/Models/Permission.php +++ b/src/permission/src/Models/Permission.php @@ -156,7 +156,15 @@ public static function findOrCreate(UnitEnum|string $name, ?string $guardName = $permission = static::findByParam(['name' => $name, 'guard_name' => $guardName]); if (! $permission) { - return static::query()->createOrFirst(['name' => $name, 'guard_name' => $guardName]); + $attributes = ['name' => $name, 'guard_name' => $guardName]; + $query = static::query(); + + if (static::isSoftDeletable()) { + // @phpstan-ignore method.notFound (SoftDeletingScope adds this method) + return $query->createOrRestore($attributes); + } + + return $query->createOrFirst($attributes); } return $permission; diff --git a/src/permission/src/Models/Role.php b/src/permission/src/Models/Role.php index fab46426f..1f9559e34 100644 --- a/src/permission/src/Models/Role.php +++ b/src/permission/src/Models/Role.php @@ -182,7 +182,14 @@ public static function findOrCreate(UnitEnum|string $name, ?string $guardName = $attributes[$teamsKey] = getPermissionsTeamId(); } - return static::query()->createOrFirst($attributes); + $query = static::query(); + + if (static::isSoftDeletable()) { + // @phpstan-ignore method.notFound (SoftDeletingScope adds this method) + return $query->createOrRestore($attributes); + } + + return $query->createOrFirst($attributes); } return $role; diff --git a/tests/Permission/Traits/HasPermissionsWithCustomModelsTest.php b/tests/Permission/Traits/HasPermissionsWithCustomModelsTest.php index 4c8f72d04..ef0b528d2 100644 --- a/tests/Permission/Traits/HasPermissionsWithCustomModelsTest.php +++ b/tests/Permission/Traits/HasPermissionsWithCustomModelsTest.php @@ -31,6 +31,25 @@ public function testItCanUseCustomModelPermission(): void $this->assertSame(Permission::class, $this->testUserPermission::class); } + public function testFindOrCreateRestoresSoftDeletedPermission(): void + { + $permission = Permission::create(['name' => 'restorable-permission']); + $permissionId = $permission->getKey(); + $this->testUserRole->givePermissionTo($permission); + $this->testUser->givePermissionTo($permission); + + $permission->delete(); + + $restoredPermission = Permission::findOrCreate('restorable-permission'); + + $this->assertSame($permissionId, $restoredPermission->getKey()); + $this->assertFalse($restoredPermission->trashed()); + $this->assertNull($restoredPermission->deleted_at); + $this->assertSame(1, Permission::withTrashed()->where('name', 'restorable-permission')->count()); + $this->assertTrue($this->testUserRole->hasPermissionTo($restoredPermission)); + $this->assertTrue($this->testUser->fresh()->hasPermissionTo($restoredPermission)); + } + public function testItCanUseCustomFieldsFromCache(): void { DB::connection()->getSchemaBuilder()->table(config('permission.table_names.roles'), function ($table): void { diff --git a/tests/Permission/Traits/HasRolesWithCustomModelsTest.php b/tests/Permission/Traits/HasRolesWithCustomModelsTest.php index 9bc821ea6..f4bed3265 100644 --- a/tests/Permission/Traits/HasRolesWithCustomModelsTest.php +++ b/tests/Permission/Traits/HasRolesWithCustomModelsTest.php @@ -23,6 +23,25 @@ public function testItCanUseCustomModelRole(): void $this->assertSame(Role::class, $this->testUserRole::class); } + public function testFindOrCreateRestoresSoftDeletedRole(): void + { + $role = Role::create(['name' => 'restorable-role']); + $roleId = $role->getKey(); + $role->givePermissionTo($this->testUserPermission); + $this->testUser->assignRole($role); + + $role->delete(); + + $restoredRole = Role::findOrCreate('restorable-role'); + + $this->assertSame($roleId, $restoredRole->getKey()); + $this->assertFalse($restoredRole->trashed()); + $this->assertNull($restoredRole->deleted_at); + $this->assertSame(1, Role::withTrashed()->where('name', 'restorable-role')->count()); + $this->assertTrue($restoredRole->hasPermissionTo($this->testUserPermission)); + $this->assertTrue($this->testUser->fresh()->hasRole($restoredRole)); + } + public function testItDoesNotDetachPermissionsWhenSoftDeleting(): void { $this->testUserRole->givePermissionTo($this->testUserPermission);