From f5d0afff1de78803c7e41b7a26fa0c3db66dd6cf Mon Sep 17 00:00:00 2001 From: Jason Vertucio Date: Tue, 29 Sep 2026 07:30:59 -0400 Subject: [PATCH 1/9] refactor: Update cache key for permissions and enhance getPermissions method --- src/Services/PermissionRegistrar.php | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/src/Services/PermissionRegistrar.php b/src/Services/PermissionRegistrar.php index 2bb91b0..9c11bfa 100644 --- a/src/Services/PermissionRegistrar.php +++ b/src/Services/PermissionRegistrar.php @@ -26,7 +26,7 @@ class PermissionRegistrar /** * The cache key for storing permissions. */ - protected string $cacheKey = 'keystone.permissions.all'; + protected string $cacheKey = 'keystone.permissions.all.v2'; public function __construct(CacheRepository $cache) { @@ -86,13 +86,19 @@ public function registerPermissions(Gate $gate): void */ protected function getPermissions() { - return $this->cache->remember( + $rows = $this->cache->remember( $this->cacheKey, $this->cacheExpiration(), - function () { - return KeystonePermission::withoutTenant()->get(); - } + fn() => KeystonePermission::withoutTenant() + ->get(['name', 'guard_name']) + ->map(fn($permission) => [ + 'name' => $permission->name, + 'guard_name' => $permission->guard_name, + ]) + ->all() ); + + return collect($rows); } /** From 0412797eff1bf4ab304d4e0eaa37e138b6483f4e Mon Sep 17 00:00:00 2001 From: Jason Vertucio Date: Tue, 29 Sep 2026 07:30:59 -0400 Subject: [PATCH 2/9] refactor: Update cache key for permissions and enhance getPermissions method --- src/Services/PermissionRegistrar.php | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/src/Services/PermissionRegistrar.php b/src/Services/PermissionRegistrar.php index 2bb91b0..10a070f 100644 --- a/src/Services/PermissionRegistrar.php +++ b/src/Services/PermissionRegistrar.php @@ -26,7 +26,7 @@ class PermissionRegistrar /** * The cache key for storing permissions. */ - protected string $cacheKey = 'keystone.permissions.all'; + protected string $cacheKey = 'keystone.permissions.all.v2'; public function __construct(CacheRepository $cache) { @@ -86,13 +86,19 @@ public function registerPermissions(Gate $gate): void */ protected function getPermissions() { - return $this->cache->remember( + $rows = $this->cache->remember( $this->cacheKey, $this->cacheExpiration(), - function () { - return KeystonePermission::withoutTenant()->get(); - } + fn () => KeystonePermission::withoutTenant() + ->get(['name', 'guard_name']) + ->map(fn ($permission) => [ + 'name' => $permission->name, + 'guard_name' => $permission->guard_name, + ]) + ->all() ); + + return collect($rows); } /** From 89a3d08a1ea1c8eb4bda77d958f9c7a21ff5239b Mon Sep 17 00:00:00 2001 From: Jason Vertucio Date: Tue, 29 Sep 2026 08:53:57 -0400 Subject: [PATCH 3/9] feat!: fix API user targeting, additive assign endpoints, literal role checks Security: - /api/users/{user}/... routes acted on the authenticated caller instead of the user in the URL (Authenticatable type-hint can't be route-bound), so any holder of assign-roles could grant themselves super-admin. The controller now resolves {user} against the configured user model, returns 404 for unknown IDs, and 404s cross-tenant targets for tenant callers. Breaking: - POST assign endpoints and AuthorizationService::assignRolesToUser / assignPermissionsToUser now add instead of replace. New PUT routes and syncRolesForUser / syncPermissionsForUser replace. - hasRole / hasAnyRole / hasAllRoles are literal for super-admins; the bypass remains in permission checks, Gate, middleware, and AuthorizationService. - KeystonePermission::forTenant() returns only that tenant's rows and respects the tenant scope, matching KeystoneRole::forTenant(). Fixed: - role:/permission: middleware throw AuthenticationException for guests instead of redirecting to an undefined login route. - Permission cache is cleared automatically on permission create/update/delete; removed dead per-user cache clearing. - PermissionRegistrarTest updated for the keystone.permissions.all.v2 key. Co-Authored-By: Claude Opus 5.5 --- README.md | 39 +++++- docs/multi-tenancy.md | 30 ++++ routes/api.php | 16 ++- src/Console/Commands/AssignRoleCommand.php | 2 +- .../Controllers/RolePermissionController.php | 112 ++++++++++++++- src/Http/Middleware/EnsureHasPermission.php | 3 +- src/Http/Middleware/EnsureHasRole.php | 3 +- src/Models/KeystonePermission.php | 28 ++-- src/Models/KeystoneRole.php | 19 ++- src/Services/AuthorizationService.php | 32 +++-- .../AuthorizationServiceInterface.php | 14 +- src/Traits/HasKeystone.php | 23 +-- tests/Feature/AssignRoleCommandTest.php | 43 ++++++ tests/Feature/KeystoneTest.php | 63 +++++++++ tests/Feature/RolePermissionApiTest.php | 119 ++++++++++++++++ tests/Feature/SingleTenantApiTest.php | 34 +++++ tests/Feature/UserRouteBindingTest.php | 131 ++++++++++++++++++ tests/Unit/HasKeystoneTraitTest.php | 37 +++++ .../Unit/Models/MultiTenantPermissionTest.php | 60 ++++++++ tests/Unit/Models/MultiTenantRoleTest.php | 60 ++++++++ .../Services/AuthorizationServiceTest.php | 100 +++++++++++++ .../Unit/Services/PermissionRegistrarTest.php | 57 +++++++- .../Traits/MultiTenantRoleAssignmentTest.php | 28 ++++ 23 files changed, 986 insertions(+), 67 deletions(-) create mode 100644 tests/Feature/AssignRoleCommandTest.php create mode 100644 tests/Feature/UserRouteBindingTest.php create mode 100644 tests/Unit/Services/AuthorizationServiceTest.php diff --git a/README.md b/README.md index afe36f7..06afb0e 100644 --- a/README.md +++ b/README.md @@ -188,9 +188,12 @@ class AdminController extends Controller // Get all roles $roles = $this->roleService->getAllWithPermissions(); - // Assign roles to user + // Add roles to user (keeps existing roles) $this->authService->assignRolesToUser($user, ['admin', 'editor']); + // Or replace the user's roles with exactly this set + $this->authService->syncRolesForUser($user, ['editor']); + // Check if user has role if ($this->authService->userHasRole($user, 'admin')) { // User is admin @@ -294,6 +297,11 @@ if (auth()->user()->isSuperAdmin()) { } ``` +> **Super-admins and role checks:** super-admins pass every *permission* check (`hasPermissionTo()`, +> `can()`, the `permission:` middleware) and the `role:` middleware, but `hasRole()` / +> `hasAnyRole()` / `hasAllRoles()` are literal. A super-admin only "has" the roles actually +> assigned to them. Use `isSuperAdmin()` or a permission check when you mean "may do anything". + > **Note:** `can()` and Laravel's `Gate` rely on Keystone registering permissions in `KeystoneServiceProvider::registerPermissionsWithGate()`, which is skipped when running in the console (e.g. `php artisan tinker`) to avoid registration during install/migration — it still runs normally during HTTP requests and the test suite. If you're debugging permissions in `tinker` and `can()` always returns `false`, this is why; use `hasPermissionTo()` / `hasRole()` directly instead, which don't depend on Gate registration. #### Service Layer Approach (Recommended for Controllers) @@ -327,7 +335,7 @@ curl -X GET http://localhost/api/roles \ -H "Accept: application/json" ``` -**Assign Role to User:** +**Add a Role to a User** (keeps the user's other roles): ```bash curl -X POST http://localhost/api/users/1/roles \ @@ -336,6 +344,18 @@ curl -X POST http://localhost/api/users/1/roles \ -d '{"roles": ["admin"]}' ``` +**Replace a User's Roles** (`PUT`; send `[]` to remove all): + +```bash +curl -X PUT http://localhost/api/users/1/roles \ + -H "Authorization: Bearer YOUR_TOKEN" \ + -H "Content-Type: application/json" \ + -d '{"roles": ["editor"]}' +``` + +The same `POST` adds / `PUT` replaces pattern applies to `/api/users/{user}/permissions` and +`/api/roles/{role}/permissions`. + ## Architecture ### Service Layer @@ -347,7 +367,7 @@ All role and permission operations go through dedicated services: - **PermissionService** - Permission CRUD and queries - `getAllWithRoles()`, `create()`, `delete()`, `syncToUser()` - **AuthorizationService** - High-level authorization operations - - `assignRolesToUser()`, `assignPermissionsToUser()`, `userHasRole()`, `userHasPermission()` + - `assignRolesToUser()`, `assignPermissionsToUser()` (add), `syncRolesForUser()`, `syncPermissionsForUser()` (replace), `userHasRole()`, `userHasPermission()` All services are registered in Laravel's service container with interface bindings and convenient aliases: @@ -423,6 +443,19 @@ $manager = KeystoneRole::create([ ]); ``` +#### Tenant Query Scopes + +Both `KeystoneRole` and `KeystonePermission` provide the same scopes. The automatic tenant +scope always applies, so a tenant user already sees their tenant's rows plus global rows. These +scopes narrow that further. They never widen it. + +```php +KeystoneRole::global()->get(); // global only (tenant_id = NULL) +KeystoneRole::tenantSpecific()->get(); // tenant-owned only (tenant_id NOT NULL) +KeystoneRole::forTenant($tenantId)->get(); // that tenant's rows only — no globals +KeystoneRole::withoutTenant()->forTenant($otherId)->get(); // explicit cross-tenant read +``` + #### Super-Admin Operations ```php diff --git a/docs/multi-tenancy.md b/docs/multi-tenancy.md index ca109f0..a3f277d 100644 --- a/docs/multi-tenancy.md +++ b/docs/multi-tenancy.md @@ -275,6 +275,33 @@ This means: - You don't need to manually add `where('tenant_id', ...)` to every query - The filtering happens automatically at the model level +### Tenant Query Scopes + +Both models share these scopes. Each one narrows the query and combines with the automatic tenant +scope above; none of them removes it: + +| Scope | Returns | +|---|---| +| `global()` | Rows with `tenant_id = NULL` | +| `tenantSpecific()` | Rows with a non-null `tenant_id` | +| `forTenant($tenantId)` | Rows with `tenant_id = $tenantId` only. Global rows are **not** included | +| `withoutTenant()` | Removes the automatic tenant scope (cross-tenant reads) | + +Because the automatic scope still applies, a tenant A user calling `forTenant($tenantB)` gets no +rows. To read another tenant's roles or permissions, opt in explicitly: + +```php +KeystonePermission::withoutTenant()->forTenant($tenantB)->get(); +``` + +To get one tenant's rows **plus** globals for an arbitrary tenant: + +```php +KeystonePermission::withoutTenant() + ->where(fn ($q) => $q->where('tenant_id', $tenantId)->orWhereNull('tenant_id')) + ->get(); +``` + ### Auto-Population on Creation When creating roles or permissions, the `tenant_id` is automatically populated from the authenticated user: @@ -363,6 +390,9 @@ if ($user->isSuperAdmin()) { $tenantRoles = KeystoneRole::all(); } +// Role checks are literal, even for super-admins: +$user->hasRole('manager'); // true only if the user actually holds `manager` + // Or use the method directly if ($user->canBypassPermissions()) { // Same as isSuperAdmin() diff --git a/routes/api.php b/routes/api.php index cb0ae7d..f1f7fca 100644 --- a/routes/api.php +++ b/routes/api.php @@ -46,22 +46,34 @@ ->middleware('permission:delete-permissions') ->name('api.permissions.destroy'); - // Assign roles and permissions to users + // Assign (POST adds) or sync (PUT replaces) roles and permissions for users Route::post('/users/{user}/roles', [RolePermissionController::class, 'assignRoles']) ->middleware('permission:assign-roles') ->name('api.users.roles.assign'); + Route::put('/users/{user}/roles', [RolePermissionController::class, 'syncRoles']) + ->middleware('permission:assign-roles') + ->name('api.users.roles.sync'); + Route::post('/users/{user}/permissions', [RolePermissionController::class, 'assignPermissions']) ->middleware('permission:assign-permissions') ->name('api.users.permissions.assign'); + Route::put('/users/{user}/permissions', [RolePermissionController::class, 'syncPermissions']) + ->middleware('permission:assign-permissions') + ->name('api.users.permissions.sync'); + // Get user roles and permissions Route::get('/users/{user}/roles-permissions', [RolePermissionController::class, 'userRolesPermissions']) ->middleware('permission:view-users') ->name('api.users.roles-permissions'); - // Assign permissions to roles + // Assign (POST adds) or sync (PUT replaces) permissions for roles Route::post('/roles/{role}/permissions', [RolePermissionController::class, 'assignPermissionsToRole']) ->middleware('permission:assign-permissions') ->name('api.roles.permissions.assign'); + + Route::put('/roles/{role}/permissions', [RolePermissionController::class, 'syncRolePermissions']) + ->middleware('permission:assign-permissions') + ->name('api.roles.permissions.sync'); }); diff --git a/src/Console/Commands/AssignRoleCommand.php b/src/Console/Commands/AssignRoleCommand.php index df25760..f9fdfea 100644 --- a/src/Console/Commands/AssignRoleCommand.php +++ b/src/Console/Commands/AssignRoleCommand.php @@ -57,7 +57,7 @@ public function handle(): int $action = 'removed'; } elseif ($this->option('sync')) { // Replace all roles - $this->authorizationService->assignRolesToUser($user, $roles); + $this->authorizationService->syncRolesForUser($user, $roles); $action = 'synced'; } else { // Add roles (default behavior) diff --git a/src/Http/Controllers/RolePermissionController.php b/src/Http/Controllers/RolePermissionController.php index 1363f24..d7e8240 100644 --- a/src/Http/Controllers/RolePermissionController.php +++ b/src/Http/Controllers/RolePermissionController.php @@ -2,6 +2,7 @@ namespace BSPDX\Keystone\Http\Controllers; +use App\Models\User; use BSPDX\Keystone\Models\KeystonePermission; use BSPDX\Keystone\Models\KeystoneRole; use BSPDX\Keystone\Services\Contracts\AuthorizationServiceInterface; @@ -108,10 +109,12 @@ public function createPermission(Request $request): JsonResponse } /** - * Assign roles to a user. + * Add roles to a user, keeping the roles they already hold. */ - public function assignRoles(Request $request, Authenticatable $user): JsonResponse + public function assignRoles(Request $request, string $user): JsonResponse { + $user = $this->resolveUser($user); + $validated = $request->validate([ 'roles' => ['required', 'array'], 'roles.*' => ['string', 'exists:roles,name'], @@ -130,10 +133,36 @@ public function assignRoles(Request $request, Authenticatable $user): JsonRespon } /** - * Assign permissions to a user. + * Replace a user's roles with exactly the given set. + */ + public function syncRoles(Request $request, string $user): JsonResponse + { + $user = $this->resolveUser($user); + + $validated = $request->validate([ + 'roles' => ['present', 'array'], + 'roles.*' => ['string', 'exists:roles,name'], + ]); + + $this->authorizationService->syncRolesForUser($user, $validated['roles']); + + return response()->json([ + 'message' => 'Roles synced successfully.', + 'user' => [ + 'id' => $user->id, + 'name' => $user->name, + 'roles' => $user->roles->pluck('name'), + ], + ]); + } + + /** + * Add direct permissions to a user, keeping the ones they already hold. */ - public function assignPermissions(Request $request, Authenticatable $user): JsonResponse + public function assignPermissions(Request $request, string $user): JsonResponse { + $user = $this->resolveUser($user); + $validated = $request->validate([ 'permissions' => ['required', 'array'], 'permissions.*' => ['string', 'exists:permissions,name'], @@ -152,7 +181,31 @@ public function assignPermissions(Request $request, Authenticatable $user): Json } /** - * Assign permissions to a role. + * Replace a user's direct permissions with exactly the given set. + */ + public function syncPermissions(Request $request, string $user): JsonResponse + { + $user = $this->resolveUser($user); + + $validated = $request->validate([ + 'permissions' => ['present', 'array'], + 'permissions.*' => ['string', 'exists:permissions,name'], + ]); + + $this->authorizationService->syncPermissionsForUser($user, $validated['permissions']); + + return response()->json([ + 'message' => 'Permissions synced successfully.', + 'user' => [ + 'id' => $user->id, + 'name' => $user->name, + 'permissions' => $this->permissionService->getAllUserPermissions($user)->pluck('name'), + ], + ]); + } + + /** + * Add permissions to a role, keeping the ones it already holds. */ public function assignPermissionsToRole(Request $request, KeystoneRole $role): JsonResponse { @@ -161,10 +214,28 @@ public function assignPermissionsToRole(Request $request, KeystoneRole $role): J 'permissions.*' => ['string', 'exists:permissions,name'], ]); - $role = $this->roleService->syncPermissions($role, $validated['permissions']); + $this->permissionService->assignToRole($role, $validated['permissions']); return response()->json([ 'message' => 'Permissions assigned to role successfully.', + 'role' => $role->load('permissions'), + ]); + } + + /** + * Replace a role's permissions with exactly the given set. + */ + public function syncRolePermissions(Request $request, KeystoneRole $role): JsonResponse + { + $validated = $request->validate([ + 'permissions' => ['present', 'array'], + 'permissions.*' => ['string', 'exists:permissions,name'], + ]); + + $role = $this->roleService->syncPermissions($role, $validated['permissions']); + + return response()->json([ + 'message' => 'Role permissions synced successfully.', 'role' => $role, ]); } @@ -172,8 +243,10 @@ public function assignPermissionsToRole(Request $request, KeystoneRole $role): J /** * Get user's roles and permissions. */ - public function userRolesPermissions(Authenticatable $user): JsonResponse + public function userRolesPermissions(string $user): JsonResponse { + $user = $this->resolveUser($user); + return response()->json([ 'user' => [ 'id' => $user->id, @@ -221,4 +294,29 @@ public function deletePermission(KeystonePermission $permission): JsonResponse 'message' => 'Permission deleted successfully.', ]); } + + /** + * Resolve the {user} route segment to the configured user model. + * + * Resolved here rather than via a global Route::bind('user') so the + * consuming app's own {user} bindings are left alone. Users in another + * tenant are reported as missing (404) rather than forbidden. + */ + private function resolveUser(string $id): Authenticatable + { + $userModel = config('keystone.user.model') + ?? config('auth.providers.users.model', User::class); + + $user = $userModel::findOrFail($id); + + $callerTenant = auth()->user()?->tenant_id; + + if (config('keystone.features.multi_tenant', false) + && $callerTenant !== null + && $user->tenant_id !== $callerTenant) { + abort(404); + } + + return $user; + } } diff --git a/src/Http/Middleware/EnsureHasPermission.php b/src/Http/Middleware/EnsureHasPermission.php index adf78d9..691a32a 100644 --- a/src/Http/Middleware/EnsureHasPermission.php +++ b/src/Http/Middleware/EnsureHasPermission.php @@ -4,6 +4,7 @@ use BSPDX\Keystone\Services\Contracts\AuthorizationServiceInterface; use Closure; +use Illuminate\Auth\AuthenticationException; use Illuminate\Http\Request; use Symfony\Component\HttpFoundation\Response; @@ -30,7 +31,7 @@ public function __construct(AuthorizationServiceInterface $authorizationService) public function handle(Request $request, Closure $next, string ...$permissions): Response { if (! $request->user()) { - return redirect()->route('login'); + throw new AuthenticationException('Unauthenticated.'); } // Super admins bypass all permission checks diff --git a/src/Http/Middleware/EnsureHasRole.php b/src/Http/Middleware/EnsureHasRole.php index bd45240..76ca13d 100644 --- a/src/Http/Middleware/EnsureHasRole.php +++ b/src/Http/Middleware/EnsureHasRole.php @@ -4,6 +4,7 @@ use BSPDX\Keystone\Services\Contracts\AuthorizationServiceInterface; use Closure; +use Illuminate\Auth\AuthenticationException; use Illuminate\Http\Request; use Symfony\Component\HttpFoundation\Response; @@ -30,7 +31,7 @@ public function __construct(AuthorizationServiceInterface $authorizationService) public function handle(Request $request, Closure $next, string ...$roles): Response { if (! $request->user()) { - return redirect()->route('login'); + throw new AuthenticationException('Unauthenticated.'); } // Super admins bypass all role checks diff --git a/src/Models/KeystonePermission.php b/src/Models/KeystonePermission.php index 9c6701d..2948228 100644 --- a/src/Models/KeystonePermission.php +++ b/src/Models/KeystonePermission.php @@ -2,6 +2,7 @@ namespace BSPDX\Keystone\Models; +use BSPDX\Keystone\Services\PermissionRegistrar; use Carbon\Carbon; use Illuminate\Database\Eloquent\Builder; use Illuminate\Database\Eloquent\Model; @@ -87,6 +88,10 @@ protected static function booted(): void } }); + // Keep the registrar's cached permission list in sync with the table + static::saved(fn () => app(PermissionRegistrar::class)->forgetCachedPermissions()); + static::deleted(fn () => app(PermissionRegistrar::class)->forgetCachedPermissions()); + // Auto-set tenant_id and guard_name when creating permissions static::creating(function ($permission) { // Set guard_name if not provided @@ -133,7 +138,6 @@ public function assignRole(...$roles): self $roleModels = $this->convertToRoleModels($roles); $this->roles()->syncWithoutDetaching($roleModels->pluck('id')); - $this->forgetCachedPermissions(); return $this; } @@ -149,7 +153,6 @@ public function syncRoles(...$roles): self $roleModels = $this->convertToRoleModels($roles); $this->roles()->sync($roleModels->pluck('id')); - $this->forgetCachedPermissions(); return $this; } @@ -165,7 +168,6 @@ public function removeRole(...$roles): self $roleModels = $this->convertToRoleModels($roles); $this->roles()->detach($roleModels->pluck('id')); - $this->forgetCachedPermissions(); return $this; } @@ -242,15 +244,6 @@ protected function convertToRoleModels(array $roles): Collection }); } - /** - * Clear cached permissions for all users with this permission. - */ - protected function forgetCachedPermissions(): void - { - // TODO: Implement cache clearing when caching layer is added - // For now, this is a placeholder for future caching implementation - } - /** * Check if this is a global permission (accessible across all tenants). */ @@ -307,17 +300,14 @@ public function scopeTenantSpecific(Builder $query): Builder } /** - * Scope a query to return permissions for a specific tenant. - * Includes both global and tenant-specific permissions. + * Scope a query to permissions belonging to a specific tenant only. + * Global permissions (tenant_id = NULL) are excluded; use global() for those. + * Respects the tenant global scope — chain after withoutTenant() for cross-tenant reads. * * @param string|int $tenantId */ public function scopeForTenant(Builder $query, $tenantId): Builder { - return $query->withoutGlobalScope('tenant') - ->where(function ($q) use ($tenantId) { - $q->where('tenant_id', $tenantId) - ->orWhereNull('tenant_id'); - }); + return $query->where($query->getModel()->getTable().'.tenant_id', $tenantId); } } diff --git a/src/Models/KeystoneRole.php b/src/Models/KeystoneRole.php index 60854d8..73b25b3 100644 --- a/src/Models/KeystoneRole.php +++ b/src/Models/KeystoneRole.php @@ -9,6 +9,17 @@ use Illuminate\Database\Eloquent\Relations\MorphToMany; use Illuminate\Support\Collection; +/** + * KeystoneRole Model + * + * Represents a role in the multi-tenant RBAC system. + * Supports both global roles (tenant_id = NULL) and tenant-specific roles. + * + * @method static Builder withoutTenant() + * @method static Builder global() + * @method static Builder tenantSpecific() + * @method static Builder forTenant($tenantId) + */ class KeystoneRole extends Model { /** @@ -253,10 +264,14 @@ public function scopeTenantSpecific(Builder $query): Builder } /** - * Scope a query to a specific tenant. + * Scope a query to roles belonging to a specific tenant only. + * Global roles (tenant_id = NULL) are excluded; use global() for those. + * Respects the tenant global scope — chain after withoutTenant() for cross-tenant reads. + * + * @param string|int $tenantId */ public function scopeForTenant(Builder $query, $tenantId): Builder { - return $query->where('tenant_id', $tenantId); + return $query->where($query->getModel()->getTable().'.tenant_id', $tenantId); } } diff --git a/src/Services/AuthorizationService.php b/src/Services/AuthorizationService.php index 36650ca..e7c509a 100644 --- a/src/Services/AuthorizationService.php +++ b/src/Services/AuthorizationService.php @@ -8,17 +8,33 @@ class AuthorizationService implements AuthorizationServiceInterface { /** - * Assign roles to a user. + * Add roles to a user, keeping the roles they already hold. */ public function assignRolesToUser(Authenticatable $user, array $roles): void { - $user->syncRoles($roles); + $user->assignRole($roles); } /** - * Assign permissions directly to a user. + * Add direct permissions to a user, keeping the ones they already hold. */ public function assignPermissionsToUser(Authenticatable $user, array $permissions): void + { + $user->givePermissionTo($permissions); + } + + /** + * Replace a user's roles with exactly the given set (empty clears them). + */ + public function syncRolesForUser(Authenticatable $user, array $roles): void + { + $user->syncRoles($roles); + } + + /** + * Replace a user's direct permissions with exactly the given set (empty clears them). + */ + public function syncPermissionsForUser(Authenticatable $user, array $permissions): void { $user->syncPermissions($permissions); } @@ -28,6 +44,11 @@ public function assignPermissionsToUser(Authenticatable $user, array $permission */ public function userHasRole(Authenticatable $user, string|array $roles): bool { + // Check for super admin bypass (the trait's role checks are literal) + if ($this->userCanBypassPermissions($user)) { + return true; + } + return $user->hasAnyRole($roles); } @@ -57,7 +78,6 @@ public function userHasAnyRole(Authenticatable $user, string|array $roles): bool return true; } - // Delegate to Spatie's HasRoles trait return $user->hasAnyRole($roles); } @@ -71,7 +91,6 @@ public function userHasAllRoles(Authenticatable $user, string|array $roles): boo return true; } - // Delegate to Spatie's HasRoles trait return $user->hasAllRoles($roles); } @@ -85,7 +104,6 @@ public function userHasAnyPermission(Authenticatable $user, string|array $permis return true; } - // Delegate to Spatie's HasRoles trait return $user->hasAnyPermission($permissions); } @@ -99,7 +117,6 @@ public function userHasAllPermissions(Authenticatable $user, string|array $permi return true; } - // Delegate to Spatie's HasRoles trait return $user->hasAllPermissions($permissions); } @@ -113,7 +130,6 @@ public function userHasDirectPermission(Authenticatable $user, string $permissio return true; } - // Delegate to Spatie's HasPermissions trait return $user->hasDirectPermission($permission); } } diff --git a/src/Services/Contracts/AuthorizationServiceInterface.php b/src/Services/Contracts/AuthorizationServiceInterface.php index 532e906..6e2903d 100644 --- a/src/Services/Contracts/AuthorizationServiceInterface.php +++ b/src/Services/Contracts/AuthorizationServiceInterface.php @@ -7,15 +7,25 @@ interface AuthorizationServiceInterface { /** - * Assign roles to a user. + * Add roles to a user, keeping the roles they already hold. */ public function assignRolesToUser(Authenticatable $user, array $roles): void; /** - * Assign permissions directly to a user. + * Add direct permissions to a user, keeping the ones they already hold. */ public function assignPermissionsToUser(Authenticatable $user, array $permissions): void; + /** + * Replace a user's roles with exactly the given set (empty clears them). + */ + public function syncRolesForUser(Authenticatable $user, array $roles): void; + + /** + * Replace a user's direct permissions with exactly the given set (empty clears them). + */ + public function syncPermissionsForUser(Authenticatable $user, array $permissions): void; + /** * Check if user has a role. */ diff --git a/src/Traits/HasKeystone.php b/src/Traits/HasKeystone.php index b682e41..3e1be8c 100644 --- a/src/Traits/HasKeystone.php +++ b/src/Traits/HasKeystone.php @@ -7,7 +7,6 @@ use Illuminate\Database\Eloquent\Builder; use Illuminate\Database\Eloquent\Relations\MorphToMany; use Illuminate\Support\Collection; -use Illuminate\Support\Facades\Cache; use Illuminate\Support\Facades\DB; trait HasKeystone @@ -105,7 +104,6 @@ public function assignRole(...$roles): self } $this->roles()->syncWithoutDetaching($pivotData); - $this->forgetCachedPermissions(); $this->unsetRelation('roles'); // Force reload of roles relationship return $this; @@ -130,7 +128,6 @@ public function removeRole(...$roles): self $query->delete(); - $this->forgetCachedPermissions(); $this->unsetRelation('roles'); // Force reload of roles relationship return $this; @@ -186,7 +183,6 @@ public function givePermissionTo(...$permissions): self } $this->permissions()->syncWithoutDetaching($pivotData); - $this->forgetCachedPermissions(); $this->unsetRelation('permissions'); // Force reload of permissions relationship return $this; @@ -210,7 +206,6 @@ public function revokePermissionTo(...$permissions): self $query->delete(); - $this->forgetCachedPermissions(); $this->unsetRelation('permissions'); // Force reload of permissions relationship return $this; @@ -247,14 +242,14 @@ public function syncPermissions(...$permissions): self // ============================================ /** - * Check if user has a specific role + * Check if user has a specific role. + * + * Role checks are literal: super-admins only "have" roles they actually hold. + * The super-admin bypass applies to permission checks, the Gate, the + * role/permission middleware, and AuthorizationService — not here. */ public function hasRole($roles, string $guard = 'web'): bool { - if ($this->isSuperAdmin()) { - return true; - } - if (is_string($roles)) { return $this->roles ->where('guard_name', $guard) @@ -444,14 +439,6 @@ protected function convertToPermissionModels($permissions): Collection }); } - /** - * Clear cached permissions for this user - */ - protected function forgetCachedPermissions(): void - { - Cache::forget("user_permissions_{$this->id}"); - } - // ============================================ // ADMIN METHODS // ============================================ diff --git a/tests/Feature/AssignRoleCommandTest.php b/tests/Feature/AssignRoleCommandTest.php new file mode 100644 index 0000000..eb354f2 --- /dev/null +++ b/tests/Feature/AssignRoleCommandTest.php @@ -0,0 +1,43 @@ + 'editor']); + KeystoneRole::create(['name' => 'admin']); + } + + #[Test] + public function default_mode_adds_roles() + { + $user = User::factory()->create(); + $user->assignRole('editor'); + + $this->artisan('keystone:assign-role', ['user' => $user->id, 'role' => ['admin']]) + ->assertSuccessful(); + + $this->assertEqualsCanonicalizing(['editor', 'admin'], $user->fresh()->roles->pluck('name')->all()); + } + + #[Test] + public function sync_option_replaces_roles() + { + $user = User::factory()->create(); + $user->assignRole('editor'); + + $this->artisan('keystone:assign-role', ['user' => $user->id, 'role' => ['admin'], '--sync' => true]) + ->assertSuccessful(); + + $this->assertSame(['admin'], $user->fresh()->roles->pluck('name')->all()); + } +} diff --git a/tests/Feature/KeystoneTest.php b/tests/Feature/KeystoneTest.php index dc17f59..e43782e 100644 --- a/tests/Feature/KeystoneTest.php +++ b/tests/Feature/KeystoneTest.php @@ -6,6 +6,7 @@ use BSPDX\Keystone\Models\KeystonePermission; use BSPDX\Keystone\Models\KeystoneRole; use BSPDX\Keystone\Services\Contracts\CacheServiceInterface; +use Illuminate\Auth\AuthenticationException; use Illuminate\Support\Facades\Route; use PHPUnit\Framework\Attributes\Test; use Tests\TestCase; @@ -24,6 +25,20 @@ protected function setUp(): void ->get('/test-admin-route', function () { return response()->json(['message' => 'Success']); }); + + // Routes without `auth` in front, so Keystone's own guest handling is reached + Route::middleware(['web', 'role:admin']) + ->get('/test-guest-role-route', fn () => response()->json(['message' => 'Success'])); + Route::middleware(['web', 'permission:edit-posts']) + ->get('/test-guest-permission-route', fn () => response()->json(['message' => 'Success'])); + } + + protected function tearDown(): void + { + // Reset the static guest-redirect callback so it can't leak between tests + AuthenticationException::redirectUsing(fn () => null); + + parent::tearDown(); } #[Test] @@ -125,4 +140,52 @@ public function super_admin_bypasses_permission_checks() // Super admin should bypass permission checks $this->assertTrue($user->canBypassPermissions()); } + + #[Test] + public function json_guest_gets_401_from_role_and_permission_middleware() + { + // No route named `login` exists in the test app, so this also proves + // the middleware no longer depends on one. + $this->getJson('/test-guest-role-route')->assertStatus(401); + $this->getJson('/test-guest-permission-route')->assertStatus(401); + } + + #[Test] + public function browser_guest_is_redirected_to_the_login_route_when_one_exists() + { + Route::get('/login', fn () => 'login')->name('login'); + Route::getRoutes()->refreshNameLookups(); + + $this->get('/test-guest-role-route')->assertRedirect('/login'); + $this->get('/test-guest-permission-route')->assertRedirect('/login'); + } + + #[Test] + public function browser_guest_follows_the_apps_configured_guest_redirect() + { + AuthenticationException::redirectUsing(fn () => '/sign-in'); + + $this->get('/test-guest-role-route')->assertRedirect('/sign-in'); + $this->get('/test-guest-permission-route')->assertRedirect('/sign-in'); + } + + #[Test] + public function super_admin_passes_role_middleware_without_holding_the_role() + { + KeystoneRole::create(['name' => 'super-admin']); + KeystoneRole::create(['name' => 'admin']); + $superAdmin = User::factory()->create(); + $superAdmin->assignRole('super-admin'); + + $this->actingAs($superAdmin)->getJson('/test-admin-route')->assertOk(); + } + + #[Test] + public function regular_user_without_the_role_is_forbidden_by_role_middleware() + { + KeystoneRole::create(['name' => 'admin']); + $user = User::factory()->create(); + + $this->actingAs($user)->getJson('/test-admin-route')->assertForbidden(); + } } diff --git a/tests/Feature/RolePermissionApiTest.php b/tests/Feature/RolePermissionApiTest.php index 97f7688..f3b20aa 100644 --- a/tests/Feature/RolePermissionApiTest.php +++ b/tests/Feature/RolePermissionApiTest.php @@ -30,6 +30,13 @@ protected function setUp(): void Route::get('/permissions', [RolePermissionController::class, 'permissions']) ->name('api.permissions.index'); + + Route::post('/users/{user}/roles', [RolePermissionController::class, 'assignRoles']); + Route::put('/users/{user}/roles', [RolePermissionController::class, 'syncRoles']); + Route::post('/users/{user}/permissions', [RolePermissionController::class, 'assignPermissions']); + Route::put('/users/{user}/permissions', [RolePermissionController::class, 'syncPermissions']); + Route::post('/roles/{role}/permissions', [RolePermissionController::class, 'assignPermissionsToRole']); + Route::put('/roles/{role}/permissions', [RolePermissionController::class, 'syncRolePermissions']); }); } @@ -74,4 +81,116 @@ public function permissions_endpoint_returns_created_permissions() 'name' => 'edit-posts', ]); } + + #[Test] + public function post_user_roles_adds_without_removing_existing_roles() + { + [$caller, $target] = $this->usersWithRoles(['editor', 'admin']); + $target->assignRole('editor'); + + $this->actingAs($caller)->postJson("/users/{$target->id}/roles", ['roles' => ['admin']])->assertOk(); + + $this->assertEqualsCanonicalizing(['editor', 'admin'], $target->fresh()->roles->pluck('name')->all()); + } + + #[Test] + public function post_user_roles_does_not_duplicate_an_already_held_role() + { + [$caller, $target] = $this->usersWithRoles(['editor']); + $target->assignRole('editor'); + + $this->actingAs($caller)->postJson("/users/{$target->id}/roles", ['roles' => ['editor']])->assertOk(); + + $this->assertSame(['editor'], $target->fresh()->roles->pluck('name')->all()); + } + + #[Test] + public function post_user_roles_rejects_an_empty_array() + { + [$caller, $target] = $this->usersWithRoles(['editor']); + + $this->actingAs($caller)->postJson("/users/{$target->id}/roles", ['roles' => []])->assertStatus(422); + } + + #[Test] + public function put_user_roles_replaces_roles() + { + [$caller, $target] = $this->usersWithRoles(['editor', 'admin']); + $target->assignRole('editor'); + + $this->actingAs($caller)->putJson("/users/{$target->id}/roles", ['roles' => ['admin']])->assertOk(); + + $this->assertSame(['admin'], $target->fresh()->roles->pluck('name')->all()); + } + + #[Test] + public function put_user_roles_with_empty_array_clears_roles() + { + [$caller, $target] = $this->usersWithRoles(['editor']); + $target->assignRole('editor'); + + $this->actingAs($caller)->putJson("/users/{$target->id}/roles", ['roles' => []])->assertOk(); + + $this->assertCount(0, $target->fresh()->roles); + } + + #[Test] + public function post_user_permissions_adds_and_put_replaces() + { + [$caller, $target] = $this->usersWithRoles([]); + KeystonePermission::create(['name' => 'edit-posts']); + KeystonePermission::create(['name' => 'publish-posts']); + $target->givePermissionTo('publish-posts'); + + $this->actingAs($caller)->postJson("/users/{$target->id}/permissions", ['permissions' => ['edit-posts']])->assertOk(); + $this->assertEqualsCanonicalizing( + ['publish-posts', 'edit-posts'], + $target->fresh()->permissions->pluck('name')->all() + ); + + $this->actingAs($caller)->putJson("/users/{$target->id}/permissions", ['permissions' => ['edit-posts']])->assertOk(); + $this->assertSame(['edit-posts'], $target->fresh()->permissions->pluck('name')->all()); + } + + #[Test] + public function put_user_permissions_without_the_key_is_rejected() + { + [$caller, $target] = $this->usersWithRoles([]); + + $this->actingAs($caller)->putJson("/users/{$target->id}/permissions", [])->assertStatus(422); + } + + #[Test] + public function post_role_permissions_adds_and_put_replaces() + { + $caller = User::factory()->create(); + $role = KeystoneRole::create(['name' => 'editor']); + KeystonePermission::create(['name' => 'view-posts']); + KeystonePermission::create(['name' => 'edit-posts']); + $role->givePermissionTo('view-posts'); + + $this->actingAs($caller)->postJson("/roles/{$role->id}/permissions", ['permissions' => ['edit-posts']]) + ->assertOk() + ->assertJsonCount(2, 'role.permissions'); + $this->assertEqualsCanonicalizing(['view-posts', 'edit-posts'], $role->fresh()->getPermissionNames()->all()); + + $this->actingAs($caller)->putJson("/roles/{$role->id}/permissions", ['permissions' => ['edit-posts']]) + ->assertOk() + ->assertJsonCount(1, 'role.permissions'); + $this->assertSame(['edit-posts'], $role->fresh()->getPermissionNames()->all()); + } + + /** + * Create the given roles plus a caller and a target user. + * + * @return array{0: User, 1: User} + */ + private function usersWithRoles(array $roles): array + { + foreach ($roles as $name) { + KeystoneRole::create(['name' => $name]); + } + + return [User::factory()->create(), User::factory()->create()]; + } } diff --git a/tests/Feature/SingleTenantApiTest.php b/tests/Feature/SingleTenantApiTest.php index 0c15870..0d317f1 100644 --- a/tests/Feature/SingleTenantApiTest.php +++ b/tests/Feature/SingleTenantApiTest.php @@ -3,8 +3,10 @@ namespace Tests\Feature; use App\Models\User; +use BSPDX\Keystone\Http\Controllers\RolePermissionController; use BSPDX\Keystone\Models\KeystonePermission; use BSPDX\Keystone\Models\KeystoneRole; +use Illuminate\Support\Facades\Route; use PHPUnit\Framework\Attributes\Test; use Tests\TestCase; @@ -57,4 +59,36 @@ public function full_role_and_permission_api_works_without_tenant_id_column() $user->syncRoles('editor'); $this->assertTrue($user->hasRole('editor')); } + + #[Test] + public function user_route_assigns_to_the_user_in_the_url() + { + Route::middleware(['web'])->post('/users/{user}/roles', [RolePermissionController::class, 'assignRoles']); + KeystoneRole::create(['name' => 'editor']); + [$caller, $target] = User::factory()->count(2)->create(); + + $this->actingAs($caller) + ->postJson("/users/{$target->id}/roles", ['roles' => ['editor']]) + ->assertOk(); + + $this->assertTrue($target->fresh()->hasRole('editor')); + $this->assertFalse($caller->fresh()->hasRole('editor')); + } + + #[Test] + public function post_adds_and_put_replaces_user_roles_without_tenant_columns() + { + Route::middleware(['web'])->post('/users/{user}/roles', [RolePermissionController::class, 'assignRoles']); + Route::middleware(['web'])->put('/users/{user}/roles', [RolePermissionController::class, 'syncRoles']); + KeystoneRole::create(['name' => 'editor']); + KeystoneRole::create(['name' => 'admin']); + [$caller, $target] = User::factory()->count(2)->create(); + $target->assignRole('editor'); + + $this->actingAs($caller)->postJson("/users/{$target->id}/roles", ['roles' => ['admin']])->assertOk(); + $this->assertEqualsCanonicalizing(['editor', 'admin'], $target->fresh()->roles->pluck('name')->all()); + + $this->actingAs($caller)->putJson("/users/{$target->id}/roles", ['roles' => ['admin']])->assertOk(); + $this->assertSame(['admin'], $target->fresh()->roles->pluck('name')->all()); + } } diff --git a/tests/Feature/UserRouteBindingTest.php b/tests/Feature/UserRouteBindingTest.php new file mode 100644 index 0000000..3f87847 --- /dev/null +++ b/tests/Feature/UserRouteBindingTest.php @@ -0,0 +1,131 @@ +group(function () { + Route::post('/users/{user}/roles', [RolePermissionController::class, 'assignRoles']); + Route::post('/users/{user}/permissions', [RolePermissionController::class, 'assignPermissions']); + Route::get('/users/{user}/roles-permissions', [RolePermissionController::class, 'userRolesPermissions']); + }); + + KeystoneRole::withoutTenant()->create(['name' => 'editor']); + KeystonePermission::withoutTenant()->create(['name' => 'edit-posts']); + } + + #[Test] + public function roles_are_assigned_to_the_user_in_the_url_not_the_caller() + { + [$caller, $target] = User::factory()->count(2)->create(); + + $this->actingAs($caller) + ->postJson("/users/{$target->id}/roles", ['roles' => ['editor']]) + ->assertOk() + ->assertJsonPath('user.id', $target->id); + + $this->assertTrue($target->fresh()->hasRole('editor')); + $this->assertFalse($caller->fresh()->hasRole('editor')); + } + + #[Test] + public function permissions_are_assigned_to_the_user_in_the_url_not_the_caller() + { + [$caller, $target] = User::factory()->count(2)->create(); + + $this->actingAs($caller) + ->postJson("/users/{$target->id}/permissions", ['permissions' => ['edit-posts']]) + ->assertOk() + ->assertJsonPath('user.id', $target->id); + + $this->assertTrue($target->fresh()->hasDirectPermission('edit-posts')); + $this->assertFalse($caller->fresh()->hasDirectPermission('edit-posts')); + } + + #[Test] + public function roles_permissions_shows_the_user_in_the_url() + { + [$caller, $target] = User::factory()->count(2)->create(); + $target->assignRole('editor'); + + $this->actingAs($caller) + ->getJson("/users/{$target->id}/roles-permissions") + ->assertOk() + ->assertJsonPath('user.id', $target->id) + ->assertJsonPath('user.roles.0.name', 'editor'); + } + + #[Test] + public function unknown_user_returns_404() + { + $caller = User::factory()->create(); + + $this->actingAs($caller) + ->postJson('/users/999999/roles', ['roles' => ['editor']]) + ->assertNotFound(); + + $this->assertFalse($caller->fresh()->hasRole('editor')); + } + + #[Test] + public function tenant_caller_cannot_reach_a_user_in_another_tenant() + { + $this->requireTenantSchema(); + config(['keystone.features.multi_tenant' => true]); + $caller = User::factory()->create(['tenant_id' => (string) Str::uuid()]); + $target = User::factory()->create(['tenant_id' => (string) Str::uuid()]); + + $this->actingAs($caller) + ->postJson("/users/{$target->id}/roles", ['roles' => ['editor']]) + ->assertNotFound(); + + $this->actingAs($caller) + ->getJson("/users/{$target->id}/roles-permissions") + ->assertNotFound(); + + $this->assertFalse($target->fresh()->hasRole('editor')); + } + + #[Test] + public function tenantless_caller_can_reach_a_user_in_any_tenant() + { + $this->requireTenantSchema(); + config(['keystone.features.multi_tenant' => true]); + $caller = User::factory()->create(['tenant_id' => null]); + $target = User::factory()->create(['tenant_id' => (string) Str::uuid()]); + + $this->actingAs($caller) + ->postJson("/users/{$target->id}/roles", ['roles' => ['editor']]) + ->assertOk(); + + $this->assertTrue($target->fresh()->hasRole('editor')); + } + + /** + * The single-tenant suite migrates without tenant_id columns. + */ + private function requireTenantSchema(): void + { + if (! Schema::hasColumn('model_has_roles', 'tenant_id')) { + $this->markTestSkipped('Requires the multi-tenant schema.'); + } + } +} diff --git a/tests/Unit/HasKeystoneTraitTest.php b/tests/Unit/HasKeystoneTraitTest.php index 56bd5d8..e57d06a 100644 --- a/tests/Unit/HasKeystoneTraitTest.php +++ b/tests/Unit/HasKeystoneTraitTest.php @@ -45,4 +45,41 @@ public function it_can_check_if_user_can_bypass_permissions() $this->assertTrue($user->canBypassPermissions()); } + + #[Test] + public function super_admin_role_checks_are_literal() + { + config(['keystone.rbac.super_admin_role' => 'super-admin']); + KeystoneRole::create(['name' => 'super-admin']); + KeystoneRole::create(['name' => 'editor']); + KeystoneRole::create(['name' => 'admin']); + + $user = User::factory()->create(); + $user->assignRole('super-admin'); + + $this->assertTrue($user->isSuperAdmin()); + $this->assertFalse($user->hasRole('nonexistent-role')); + $this->assertFalse($user->hasRole('editor')); + $this->assertTrue($user->hasRole('super-admin')); + $this->assertFalse($user->hasAnyRole('editor', 'admin')); + $this->assertFalse($user->hasAllRoles('super-admin', 'editor')); + + $user->assignRole('editor'); + + $this->assertTrue($user->hasRole('editor')); + $this->assertTrue($user->hasAllRoles('super-admin', 'editor')); + } + + #[Test] + public function super_admin_still_passes_every_permission_check() + { + KeystoneRole::create(['name' => 'super-admin']); + $user = User::factory()->create(); + $user->assignRole('super-admin'); + + $this->assertTrue($user->hasPermissionTo('anything')); + $this->assertTrue($user->hasAnyPermission('anything', 'else')); + $this->assertTrue($user->hasAllPermissions('anything', 'else')); + $this->assertTrue($user->hasDirectPermission('anything')); + } } diff --git a/tests/Unit/Models/MultiTenantPermissionTest.php b/tests/Unit/Models/MultiTenantPermissionTest.php index a097623..0dfb442 100644 --- a/tests/Unit/Models/MultiTenantPermissionTest.php +++ b/tests/Unit/Models/MultiTenantPermissionTest.php @@ -7,6 +7,7 @@ use BSPDX\Keystone\Services\Contracts\CacheServiceInterface; use Illuminate\Database\QueryException; use Illuminate\Support\Facades\Auth; +use Illuminate\Support\Str; use PHPUnit\Framework\Attributes\Test; use Tests\TestCase; @@ -190,4 +191,63 @@ public function explicit_tenant_id_overrides_auto_population() $this->assertEquals($differentTenantId, $permission->tenant_id); } + + #[Test] + public function for_tenant_returns_only_that_tenants_permissions_without_globals() + { + $tenantA = (string) Str::uuid(); + $tenantB = (string) Str::uuid(); + $this->seedTenantScopedPermissions($tenantA, $tenantB); + + $names = KeystonePermission::forTenant($tenantA)->pluck('name')->all(); + + $this->assertEqualsCanonicalizing(['a-only'], $names); + } + + #[Test] + public function for_tenant_respects_the_authenticated_users_tenant_scope() + { + $tenantA = (string) Str::uuid(); + $tenantB = (string) Str::uuid(); + $this->seedTenantScopedPermissions($tenantA, $tenantB); + + Auth::login(User::factory()->create(['tenant_id' => $tenantA])); + + $this->assertEqualsCanonicalizing(['a-only'], KeystonePermission::forTenant($tenantA)->pluck('name')->all()); + $this->assertCount(0, KeystonePermission::forTenant($tenantB)->get()); + } + + #[Test] + public function without_tenant_then_for_tenant_reads_another_tenant_explicitly() + { + $tenantA = (string) Str::uuid(); + $tenantB = (string) Str::uuid(); + $this->seedTenantScopedPermissions($tenantA, $tenantB); + + Auth::login(User::factory()->create(['tenant_id' => $tenantA])); + + $names = KeystonePermission::withoutTenant()->forTenant($tenantB)->pluck('name')->all(); + + $this->assertEqualsCanonicalizing(['b-only'], $names); + } + + #[Test] + public function global_scope_method_returns_only_global_permissions() + { + $tenantA = (string) Str::uuid(); + $tenantB = (string) Str::uuid(); + $this->seedTenantScopedPermissions($tenantA, $tenantB); + + $this->assertEqualsCanonicalizing(['everyone'], KeystonePermission::global()->pluck('name')->all()); + } + + /** + * One permission for tenant A, one for tenant B, and one global. + */ + private function seedTenantScopedPermissions(string $tenantA, string $tenantB): void + { + KeystonePermission::withoutTenant()->create(['name' => 'a-only', 'tenant_id' => $tenantA]); + KeystonePermission::withoutTenant()->create(['name' => 'b-only', 'tenant_id' => $tenantB]); + KeystonePermission::withoutTenant()->create(['name' => 'everyone', 'tenant_id' => null]); + } } diff --git a/tests/Unit/Models/MultiTenantRoleTest.php b/tests/Unit/Models/MultiTenantRoleTest.php index 2ad7a49..ca94bdf 100644 --- a/tests/Unit/Models/MultiTenantRoleTest.php +++ b/tests/Unit/Models/MultiTenantRoleTest.php @@ -7,6 +7,7 @@ use BSPDX\Keystone\Services\Contracts\CacheServiceInterface; use Illuminate\Database\QueryException; use Illuminate\Support\Facades\Auth; +use Illuminate\Support\Str; use PHPUnit\Framework\Attributes\Test; use Tests\TestCase; @@ -245,4 +246,63 @@ public function creating_role_with_explicit_tenant_id_uses_provided_value() // Should use the explicitly provided tenant_id, not the authenticated user's $this->assertEquals($differentTenantId, $role->tenant_id); } + + #[Test] + public function for_tenant_returns_only_that_tenants_roles_without_globals() + { + $tenantA = (string) Str::uuid(); + $tenantB = (string) Str::uuid(); + $this->seedTenantScopedRoles($tenantA, $tenantB); + + $names = KeystoneRole::forTenant($tenantA)->pluck('name')->all(); + + $this->assertEqualsCanonicalizing(['a-only'], $names); + } + + #[Test] + public function for_tenant_respects_the_authenticated_users_tenant_scope() + { + $tenantA = (string) Str::uuid(); + $tenantB = (string) Str::uuid(); + $this->seedTenantScopedRoles($tenantA, $tenantB); + + Auth::login(User::factory()->create(['tenant_id' => $tenantA])); + + $this->assertEqualsCanonicalizing(['a-only'], KeystoneRole::forTenant($tenantA)->pluck('name')->all()); + $this->assertCount(0, KeystoneRole::forTenant($tenantB)->get()); + } + + #[Test] + public function without_tenant_then_for_tenant_reads_another_tenant_explicitly() + { + $tenantA = (string) Str::uuid(); + $tenantB = (string) Str::uuid(); + $this->seedTenantScopedRoles($tenantA, $tenantB); + + Auth::login(User::factory()->create(['tenant_id' => $tenantA])); + + $names = KeystoneRole::withoutTenant()->forTenant($tenantB)->pluck('name')->all(); + + $this->assertEqualsCanonicalizing(['b-only'], $names); + } + + #[Test] + public function global_scope_method_returns_only_global_roles() + { + $tenantA = (string) Str::uuid(); + $tenantB = (string) Str::uuid(); + $this->seedTenantScopedRoles($tenantA, $tenantB); + + $this->assertEqualsCanonicalizing(['everyone'], KeystoneRole::global()->pluck('name')->all()); + } + + /** + * One role for tenant A, one for tenant B, and one global. + */ + private function seedTenantScopedRoles(string $tenantA, string $tenantB): void + { + KeystoneRole::withoutTenant()->create(['name' => 'a-only', 'tenant_id' => $tenantA]); + KeystoneRole::withoutTenant()->create(['name' => 'b-only', 'tenant_id' => $tenantB]); + KeystoneRole::withoutTenant()->create(['name' => 'everyone', 'tenant_id' => null]); + } } diff --git a/tests/Unit/Services/AuthorizationServiceTest.php b/tests/Unit/Services/AuthorizationServiceTest.php new file mode 100644 index 0000000..29f7bcd --- /dev/null +++ b/tests/Unit/Services/AuthorizationServiceTest.php @@ -0,0 +1,100 @@ +service = app(AuthorizationServiceInterface::class); + + foreach (['editor', 'admin'] as $name) { + KeystoneRole::create(['name' => $name]); + } + foreach (['edit-posts', 'publish-posts'] as $name) { + KeystonePermission::create(['name' => $name]); + } + } + + #[Test] + public function assign_roles_to_user_keeps_existing_roles() + { + $user = User::factory()->create(); + $user->assignRole('editor'); + + $this->service->assignRolesToUser($user, ['admin']); + + $this->assertEqualsCanonicalizing(['editor', 'admin'], $user->fresh()->roles->pluck('name')->all()); + } + + #[Test] + public function assign_permissions_to_user_keeps_existing_direct_permissions() + { + $user = User::factory()->create(); + $user->givePermissionTo('publish-posts'); + + $this->service->assignPermissionsToUser($user, ['edit-posts']); + + $this->assertEqualsCanonicalizing( + ['publish-posts', 'edit-posts'], + $user->fresh()->permissions->pluck('name')->all() + ); + } + + #[Test] + public function sync_roles_for_user_replaces_roles() + { + $user = User::factory()->create(); + $user->assignRole('editor'); + + $this->service->syncRolesForUser($user, ['admin']); + + $this->assertSame(['admin'], $user->fresh()->roles->pluck('name')->all()); + } + + #[Test] + public function sync_permissions_for_user_with_empty_set_clears_direct_but_not_role_permissions() + { + KeystoneRole::where('name', 'editor')->first()->givePermissionTo('edit-posts'); + $user = User::factory()->create(); + $user->assignRole('editor'); + $user->givePermissionTo('publish-posts'); + + $this->service->syncPermissionsForUser($user, []); + + $user = $user->fresh(); + $this->assertCount(0, $user->permissions); + $this->assertTrue($user->hasPermissionTo('edit-posts')); + } + + #[Test] + public function service_role_checks_bypass_for_super_admin() + { + KeystoneRole::create(['name' => 'super-admin']); + $superAdmin = User::factory()->create(); + $superAdmin->assignRole('super-admin'); + + $this->assertTrue($this->service->userHasRole($superAdmin, 'editor')); + $this->assertTrue($this->service->userHasAnyRole($superAdmin, ['editor'])); + $this->assertTrue($this->service->userHasAllRoles($superAdmin, ['editor', 'admin'])); + } + + #[Test] + public function service_role_check_is_literal_for_other_users() + { + $user = User::factory()->create(); + + $this->assertFalse($this->service->userHasRole($user, 'editor')); + } +} diff --git a/tests/Unit/Services/PermissionRegistrarTest.php b/tests/Unit/Services/PermissionRegistrarTest.php index 99b3447..8d3018c 100644 --- a/tests/Unit/Services/PermissionRegistrarTest.php +++ b/tests/Unit/Services/PermissionRegistrarTest.php @@ -2,6 +2,8 @@ namespace Tests\Unit\Services; +use BSPDX\Keystone\Models\KeystonePermission; +use BSPDX\Keystone\Services\Contracts\PermissionServiceInterface; use BSPDX\Keystone\Services\PermissionRegistrar; use Illuminate\Contracts\Cache\Repository as CacheRepository; use Illuminate\Support\Collection; @@ -19,7 +21,7 @@ public function it_uses_cache_expiration_from_keystone_config(): void $cache = Mockery::mock(CacheRepository::class); $cache->shouldReceive('remember') ->once() - ->with('keystone.permissions.all', 12345, Mockery::type('Closure')) + ->with('keystone.permissions.all.v2', 12345, Mockery::type('Closure')) ->andReturn(new Collection); $registrar = new PermissionRegistrar($cache); @@ -33,7 +35,7 @@ public function it_reads_cache_expiration_dynamically_after_construction(): void $cache = Mockery::mock(CacheRepository::class); $cache->shouldReceive('remember') ->once() - ->with('keystone.permissions.all', 555, Mockery::type('Closure')) + ->with('keystone.permissions.all.v2', 555, Mockery::type('Closure')) ->andReturn(new Collection); // Construct BEFORE changing config to prove the TTL is read at call @@ -53,11 +55,60 @@ public function it_falls_back_to_default_expiration_when_config_missing(): void $cache = Mockery::mock(CacheRepository::class); $cache->shouldReceive('remember') ->once() - ->with('keystone.permissions.all', 86400, Mockery::type('Closure')) + ->with('keystone.permissions.all.v2', 86400, Mockery::type('Closure')) ->andReturn(new Collection); $registrar = new PermissionRegistrar($cache); $registrar->getAllPermissionNames(); } + + #[Test] + public function creating_a_permission_invalidates_the_cached_list(): void + { + $registrar = app(PermissionRegistrar::class); + $registrar->getAllPermissionNames(); // warm the cache + + KeystonePermission::create(['name' => 'publish-posts']); + + $this->assertTrue($registrar->permissionExists('publish-posts')); + } + + #[Test] + public function deleting_a_permission_invalidates_the_cached_list(): void + { + $permission = KeystonePermission::create(['name' => 'edit-posts']); + $registrar = app(PermissionRegistrar::class); + $this->assertTrue($registrar->permissionExists('edit-posts')); // warm the cache + + $permission->delete(); + + $this->assertFalse($registrar->permissionExists('edit-posts')); + $this->assertNotContains('edit-posts', $registrar->getAllPermissionNames()); + } + + #[Test] + public function renaming_a_permission_invalidates_the_cached_list(): void + { + $permission = KeystonePermission::create(['name' => 'edit-posts']); + $registrar = app(PermissionRegistrar::class); + $registrar->getAllPermissionNames(); // warm the cache + + $permission->update(['name' => 'modify-posts']); + + $names = $registrar->getAllPermissionNames(); + $this->assertContains('modify-posts', $names); + $this->assertNotContains('edit-posts', $names); + } + + #[Test] + public function permission_created_through_the_service_is_visible_immediately(): void + { + $registrar = app(PermissionRegistrar::class); + $registrar->getAllPermissionNames(); // warm the cache + + app(PermissionServiceInterface::class)->create('archive-posts'); + + $this->assertTrue($registrar->permissionExists('archive-posts')); + } } diff --git a/tests/Unit/Traits/MultiTenantRoleAssignmentTest.php b/tests/Unit/Traits/MultiTenantRoleAssignmentTest.php index be84173..47404ba 100644 --- a/tests/Unit/Traits/MultiTenantRoleAssignmentTest.php +++ b/tests/Unit/Traits/MultiTenantRoleAssignmentTest.php @@ -234,4 +234,32 @@ public function syncing_roles_respects_tenant_boundaries() $this->assertTrue($userB->hasRole('admin')); $this->assertFalse($userB->hasRole('editor')); // Tenant A role not accessible } + + #[Test] + public function super_admin_does_not_see_a_role_held_only_in_another_tenant() + { + config(['keystone.rbac.super_admin_role' => 'super-admin']); + $tenantAId = '019c17d6-1a87-71be-a6a4-718da52579e9'; + $tenantBId = '019c17d7-2b98-82cf-b7b5-82be35f4c8fa'; + + KeystoneRole::withoutTenant()->create(['name' => 'super-admin', 'tenant_id' => null]); + $editor = KeystoneRole::withoutTenant()->create(['name' => 'editor', 'tenant_id' => null]); + + $user = User::factory()->create(['tenant_id' => $tenantAId]); + $user->assignRole('super-admin'); + + // Editor assigned only under tenant B's pivot scope + DB::table('model_has_roles')->insert([ + 'role_id' => $editor->id, + 'model_type' => $user->getMorphClass(), + 'model_id' => $user->id, + 'tenant_id' => $tenantBId, + 'created_at' => now(), + 'updated_at' => now(), + ]); + + $user = $user->fresh(); + $this->assertTrue($user->isSuperAdmin()); + $this->assertFalse($user->hasRole('editor')); + } } From a49e7f26df5d58a5def9575bc8f1266ade804512 Mon Sep 17 00:00:00 2001 From: Jason Vertucio Date: Tue, 29 Sep 2026 09:52:43 -0400 Subject: [PATCH 4/9] feat: add PHPStan configuration and linting scripts to enhance code quality --- composer.json | 4 ++++ phpstan.neon | 8 ++++++++ 2 files changed, 12 insertions(+) create mode 100644 phpstan.neon diff --git a/composer.json b/composer.json index 68655fc..a6f0bd0 100644 --- a/composer.json +++ b/composer.json @@ -27,6 +27,7 @@ }, "require-dev": { "fakerphp/faker": "^1.23", + "larastan/larastan": "^3.12", "laravel/pail": "^1.2.2", "laravel/pint": "^1.24", "laravel/sail": "^1.41", @@ -67,6 +68,9 @@ "@php artisan config:clear --ansi", "./vendor/bin/phpunit -c phpunit.single-tenant.xml" ], + "lint": "pint --test", + "lint:fix": "pint", + "analyze": "phpstan analyze", "version": [ "@php -r \"echo 'Current version: '; passthru('git describe --tags --abbrev=0 2>/dev/null || echo No version tags yet');\"" ], diff --git a/phpstan.neon b/phpstan.neon new file mode 100644 index 0000000..066f966 --- /dev/null +++ b/phpstan.neon @@ -0,0 +1,8 @@ +includes: + - vendor/larastan/larastan/extension.neon + +parameters: + level: 5 + paths: + - src + From 0dafa5a7014803927e727ce92298f88c7d5d2a6c Mon Sep 17 00:00:00 2001 From: Jason Vertucio Date: Tue, 29 Sep 2026 09:58:50 -0400 Subject: [PATCH 5/9] chore: resolve all PHPStan (Larastan level 5) errors - Add relation generics to HasKeystone, KeystoneRole, and KeystonePermission so role/permission collections are typed; replace higher-order proxies with typed closures. - RolePermissionController::resolveUser() returns Model&Authenticatable and responses read attributes via getKey()/getAttribute(). - RoleService::getUserRoles() / PermissionService::getUserPermissions() load through the relation methods. - Commands: drop impossible null-coalesce/ternary fallbacks; resolveGuard() only reads --guard when the command defines it. - Analyse app/ alongside src/ so the HasKeystone trait is checked in context; document tenant_id on the test-app User model. - Fix guest-redirect test ordering broken by Laravel 13.33 installing its default login redirect when the HTTP kernel is resolved. Co-Authored-By: Claude Opus 5.5 --- app/Models/User.php | 3 ++ phpstan.neon | 1 + .../Commands/AssignPermissionCommand.php | 2 +- src/Console/Commands/AssignRoleCommand.php | 2 +- .../Concerns/InteractsWithKeystone.php | 3 +- .../Commands/UnassignPermissionCommand.php | 6 ++-- src/Console/Commands/UnassignRoleCommand.php | 4 +-- .../Controllers/RolePermissionController.php | 31 ++++++++++--------- src/Models/KeystonePermission.php | 4 ++- src/Models/KeystoneRole.php | 2 ++ src/Services/PermissionService.php | 2 +- src/Services/RoleService.php | 2 +- src/Traits/HasKeystone.php | 8 +++-- tests/Feature/KeystoneTest.php | 5 +++ 14 files changed, 47 insertions(+), 28 deletions(-) diff --git a/app/Models/User.php b/app/Models/User.php index ab11a91..54fe6d2 100644 --- a/app/Models/User.php +++ b/app/Models/User.php @@ -8,6 +8,9 @@ use Illuminate\Foundation\Auth\User as Authenticatable; use Illuminate\Notifications\Notifiable; +/** + * @property string|null $tenant_id + */ class User extends Authenticatable { /** @use HasFactory */ diff --git a/phpstan.neon b/phpstan.neon index 066f966..6f29c1c 100644 --- a/phpstan.neon +++ b/phpstan.neon @@ -5,4 +5,5 @@ parameters: level: 5 paths: - src + - app diff --git a/src/Console/Commands/AssignPermissionCommand.php b/src/Console/Commands/AssignPermissionCommand.php index caa5fbe..d54ff0a 100644 --- a/src/Console/Commands/AssignPermissionCommand.php +++ b/src/Console/Commands/AssignPermissionCommand.php @@ -185,7 +185,7 @@ protected function assignToUser(string $userIdentifier, array $permissions): int */ protected function gatherPermissions(): array { - $permissions = $this->argument('permission') ?? []; + $permissions = $this->argument('permission'); // From -P / --permission options (repeatable) if ($permissionOptions = $this->option('permission')) { diff --git a/src/Console/Commands/AssignRoleCommand.php b/src/Console/Commands/AssignRoleCommand.php index f9fdfea..4d78591 100644 --- a/src/Console/Commands/AssignRoleCommand.php +++ b/src/Console/Commands/AssignRoleCommand.php @@ -97,7 +97,7 @@ public function handle(): int */ protected function gatherRoles(): array { - $roles = $this->argument('role') ?? []; + $roles = $this->argument('role'); // From -R / --role options (repeatable) if ($roleOptions = $this->option('role')) { diff --git a/src/Console/Commands/Concerns/InteractsWithKeystone.php b/src/Console/Commands/Concerns/InteractsWithKeystone.php index 9ff127e..ef00c15 100644 --- a/src/Console/Commands/Concerns/InteractsWithKeystone.php +++ b/src/Console/Commands/Concerns/InteractsWithKeystone.php @@ -29,7 +29,8 @@ protected function getDefaultGuard(): string */ protected function resolveGuard(): ?string { - $guard = $this->option('guard'); + // Not every command using this trait defines --guard + $guard = $this->hasOption('guard') ? $this->input->getOption('guard') : null; if ($guard && ! array_key_exists($guard, config('auth.guards'))) { $this->error("Guard [{$guard}] is not defined in your auth configuration."); diff --git a/src/Console/Commands/UnassignPermissionCommand.php b/src/Console/Commands/UnassignPermissionCommand.php index 57218cb..f25cc90 100644 --- a/src/Console/Commands/UnassignPermissionCommand.php +++ b/src/Console/Commands/UnassignPermissionCommand.php @@ -109,7 +109,7 @@ protected function removeFromRole(string $roleName): int [ ['Role', $role->name], ['Guard', $role->guard_name], - ['Previous Permissions', implode(', ', $previousPermissions) ?: '(none)'], + ['Previous Permissions', implode(', ', $previousPermissions)], ['Removed Permissions', implode(', ', $permissions)], ['Current Permissions', implode(', ', $currentPermissions) ?: '(none)'], ] @@ -171,7 +171,7 @@ protected function removeFromUser(string $userIdentifier): int ['Property', 'Value'], [ ['User', $user->email], - ['Previous Direct Permissions', implode(', ', $previousPermissions) ?: '(none)'], + ['Previous Direct Permissions', implode(', ', $previousPermissions)], ['Removed Permissions', implode(', ', $permissions)], ['Current Direct Permissions', implode(', ', $currentPermissions) ?: '(none)'], ['All Permissions (incl. via roles)', count($allPermissions).' total'], @@ -191,7 +191,7 @@ protected function removeFromUser(string $userIdentifier): int */ protected function gatherPermissions(): array { - $permissions = $this->argument('permission') ?? []; + $permissions = $this->argument('permission'); // From -P / --permission options (repeatable) if ($permissionOptions = $this->option('permission')) { diff --git a/src/Console/Commands/UnassignRoleCommand.php b/src/Console/Commands/UnassignRoleCommand.php index 9138202..bc9fc24 100644 --- a/src/Console/Commands/UnassignRoleCommand.php +++ b/src/Console/Commands/UnassignRoleCommand.php @@ -70,7 +70,7 @@ public function handle(): int ['Property', 'Value'], [ ['User', $user->email], - ['Previous Roles', implode(', ', $previousRoles) ?: '(none)'], + ['Previous Roles', implode(', ', $previousRoles)], ['Removed Roles', implode(', ', $roles)], ['Current Roles', implode(', ', $currentRoles) ?: '(none)'], ] @@ -89,7 +89,7 @@ public function handle(): int */ protected function gatherRoles(): array { - $roles = $this->argument('role') ?? []; + $roles = $this->argument('role'); // From -R / --role options (repeatable) if ($roleOptions = $this->option('role')) { diff --git a/src/Http/Controllers/RolePermissionController.php b/src/Http/Controllers/RolePermissionController.php index d7e8240..c21adba 100644 --- a/src/Http/Controllers/RolePermissionController.php +++ b/src/Http/Controllers/RolePermissionController.php @@ -9,6 +9,7 @@ use BSPDX\Keystone\Services\Contracts\PermissionServiceInterface; use BSPDX\Keystone\Services\Contracts\RoleServiceInterface; use Illuminate\Contracts\Auth\Authenticatable; +use Illuminate\Database\Eloquent\Model; use Illuminate\Http\JsonResponse; use Illuminate\Http\Request; @@ -125,9 +126,9 @@ public function assignRoles(Request $request, string $user): JsonResponse return response()->json([ 'message' => 'Roles assigned successfully.', 'user' => [ - 'id' => $user->id, - 'name' => $user->name, - 'roles' => $user->roles->pluck('name'), + 'id' => $user->getKey(), + 'name' => $user->getAttribute('name'), + 'roles' => $this->roleService->getUserRoles($user)->pluck('name'), ], ]); } @@ -149,9 +150,9 @@ public function syncRoles(Request $request, string $user): JsonResponse return response()->json([ 'message' => 'Roles synced successfully.', 'user' => [ - 'id' => $user->id, - 'name' => $user->name, - 'roles' => $user->roles->pluck('name'), + 'id' => $user->getKey(), + 'name' => $user->getAttribute('name'), + 'roles' => $this->roleService->getUserRoles($user)->pluck('name'), ], ]); } @@ -173,8 +174,8 @@ public function assignPermissions(Request $request, string $user): JsonResponse return response()->json([ 'message' => 'Permissions assigned successfully.', 'user' => [ - 'id' => $user->id, - 'name' => $user->name, + 'id' => $user->getKey(), + 'name' => $user->getAttribute('name'), 'permissions' => $this->permissionService->getAllUserPermissions($user)->pluck('name'), ], ]); @@ -197,8 +198,8 @@ public function syncPermissions(Request $request, string $user): JsonResponse return response()->json([ 'message' => 'Permissions synced successfully.', 'user' => [ - 'id' => $user->id, - 'name' => $user->name, + 'id' => $user->getKey(), + 'name' => $user->getAttribute('name'), 'permissions' => $this->permissionService->getAllUserPermissions($user)->pluck('name'), ], ]); @@ -249,10 +250,10 @@ public function userRolesPermissions(string $user): JsonResponse return response()->json([ 'user' => [ - 'id' => $user->id, - 'name' => $user->name, - 'email' => $user->email, - 'roles' => $user->roles->map(fn ($role) => [ + 'id' => $user->getKey(), + 'name' => $user->getAttribute('name'), + 'email' => $user->getAttribute('email'), + 'roles' => $this->roleService->getUserRoles($user)->map(fn ($role) => [ 'id' => $role->id, 'name' => $role->name, ]), @@ -302,7 +303,7 @@ public function deletePermission(KeystonePermission $permission): JsonResponse * consuming app's own {user} bindings are left alone. Users in another * tenant are reported as missing (404) rather than forbidden. */ - private function resolveUser(string $id): Authenticatable + private function resolveUser(string $id): Model&Authenticatable { $userModel = config('keystone.user.model') ?? config('auth.providers.users.model', User::class); diff --git a/src/Models/KeystonePermission.php b/src/Models/KeystonePermission.php index 2948228..97ace21 100644 --- a/src/Models/KeystonePermission.php +++ b/src/Models/KeystonePermission.php @@ -41,7 +41,7 @@ class KeystonePermission extends Model /** * The attributes that are mass assignable. * - * @var array + * @var list */ protected $fillable = [ 'name', @@ -112,6 +112,8 @@ protected static function booted(): void /** * Get the roles that have this permission. + * + * @return BelongsToMany */ public function roles(): BelongsToMany { diff --git a/src/Models/KeystoneRole.php b/src/Models/KeystoneRole.php index 73b25b3..c4356fb 100644 --- a/src/Models/KeystoneRole.php +++ b/src/Models/KeystoneRole.php @@ -84,6 +84,8 @@ protected static function booted(): void /** * The permissions that belong to the role. + * + * @return BelongsToMany */ public function permissions(): BelongsToMany { diff --git a/src/Services/PermissionService.php b/src/Services/PermissionService.php index 4b7038a..56fbd5a 100644 --- a/src/Services/PermissionService.php +++ b/src/Services/PermissionService.php @@ -50,7 +50,7 @@ public function syncToUser(Authenticatable $user, array $permissions): void */ public function getUserPermissions(Authenticatable $user): Collection { - return $user->permissions; + return $user->permissions()->get(); } /** diff --git a/src/Services/RoleService.php b/src/Services/RoleService.php index 70553ba..cebc449 100644 --- a/src/Services/RoleService.php +++ b/src/Services/RoleService.php @@ -71,7 +71,7 @@ public function syncPermissions(KeystoneRole $role, array $permissions): Keyston */ public function getUserRoles(Authenticatable $user): Collection { - return $user->roles; + return $user->roles()->get(); } /** diff --git a/src/Traits/HasKeystone.php b/src/Traits/HasKeystone.php index 3e1be8c..4f19cc2 100644 --- a/src/Traits/HasKeystone.php +++ b/src/Traits/HasKeystone.php @@ -31,6 +31,8 @@ public function scopeRole(Builder $query, string|array $roles): Builder /** * User's roles relationship with tenant filtering + * + * @return MorphToMany */ public function roles(): MorphToMany { @@ -58,6 +60,8 @@ public function roles(): MorphToMany /** * User's direct permissions (not via roles) + * + * @return MorphToMany */ public function permissions(): MorphToMany { @@ -327,7 +331,7 @@ protected function hasPermissionViaRole($permission, string $guard = 'web'): boo return $this->roles ->where('guard_name', $guard) - ->flatMap->permissions + ->flatMap(fn (KeystoneRole $role) => $role->permissions) ->where('guard_name', $guard) ->contains('name', $permissionName); } @@ -339,7 +343,7 @@ public function getAllPermissions(): Collection { $permissions = $this->permissions; - $this->roles->each(function ($role) use (&$permissions) { + $this->roles->each(function (KeystoneRole $role) use (&$permissions) { $permissions = $permissions->merge($role->permissions); }); diff --git a/tests/Feature/KeystoneTest.php b/tests/Feature/KeystoneTest.php index e43782e..48f20da 100644 --- a/tests/Feature/KeystoneTest.php +++ b/tests/Feature/KeystoneTest.php @@ -7,6 +7,7 @@ use BSPDX\Keystone\Models\KeystoneRole; use BSPDX\Keystone\Services\Contracts\CacheServiceInterface; use Illuminate\Auth\AuthenticationException; +use Illuminate\Contracts\Http\Kernel as HttpKernel; use Illuminate\Support\Facades\Route; use PHPUnit\Framework\Attributes\Test; use Tests\TestCase; @@ -163,6 +164,10 @@ public function browser_guest_is_redirected_to_the_login_route_when_one_exists() #[Test] public function browser_guest_follows_the_apps_configured_guest_redirect() { + // Resolve the kernel first: Laravel installs its default login redirect + // when the kernel is resolved, which would overwrite ours. Real apps set + // this via redirectGuestsTo() in bootstrap/app.php, which runs after it. + $this->app->make(HttpKernel::class); AuthenticationException::redirectUsing(fn () => '/sign-in'); $this->get('/test-guest-role-route')->assertRedirect('/sign-in'); From d4410ecdf265668b8dd3fc3dde6addfc59b8c137 Mon Sep 17 00:00:00 2001 From: Jason Vertucio Date: Tue, 29 Sep 2026 10:31:59 -0400 Subject: [PATCH 6/9] fix!: resolve role/permission names in the receiver's tenant In multi-tenant mode, names passed to assignment methods were resolved in the authenticated caller's tenant scope. A tenant-less caller (global admin, console command, queued job) could attach another tenant's same-named role or permission to a user, and a name existing both in-tenant and globally resolved arbitrarily. Raised in PR #2 review (discussion r4134379038). Names now resolve in the tenant of whatever receives them (user, role, or permission): the receiver's own tenant row wins over a global row, a tenant-less receiver matches only globals, and unmatched names throw ModelNotFoundException. Model instances are used as given; single-tenant installs are unchanged. Adds KeystoneRole/KeystonePermission::findByNameForTenant(). Co-Authored-By: Claude Opus 5.5 --- docs/multi-tenancy.md | 23 +++ .../Concerns/ResolvesByNameForTenant.php | 41 ++++ src/Models/KeystonePermission.php | 6 +- src/Models/KeystoneRole.php | 33 ++-- src/Traits/HasKeystone.php | 30 +-- tests/Feature/UserRouteBindingTest.php | 23 +++ .../Unit/Traits/TenantNameResolutionTest.php | 180 ++++++++++++++++++ 7 files changed, 303 insertions(+), 33 deletions(-) create mode 100644 src/Models/Concerns/ResolvesByNameForTenant.php create mode 100644 tests/Unit/Traits/TenantNameResolutionTest.php diff --git a/docs/multi-tenancy.md b/docs/multi-tenancy.md index a3f277d..4be84fd 100644 --- a/docs/multi-tenancy.md +++ b/docs/multi-tenancy.md @@ -302,6 +302,29 @@ KeystonePermission::withoutTenant() ->get(); ``` +### Name Resolution + +When you pass a role or permission **name** (rather than a model) to an assignment method, Keystone looks it up in the tenant of whatever is *receiving* it, not in the logged-in caller's scope: + +| Call | Name is resolved in | +|---|---| +| `$user->assignRole('manager')`, `removeRole`, `syncRoles`, `givePermissionTo`, `revokePermissionTo`, `syncPermissions` | the user's tenant | +| `$role->givePermissionTo('edit')`, `syncPermissions`, `revokePermissionTo` | the role's tenant | +| `$permission->assignRole('manager')`, `syncRoles`, `removeRole` | the permission's tenant | + +Rules: + +- The tenant's own row wins over a global row with the same name. +- If the receiver has no tenant, only global rows match. +- If nothing matches, a `ModelNotFoundException` is thrown and nothing changes. +- Model instances are always used exactly as given. + +This means a global admin, console command, or queued job assigning `manager` to a tenant B user always gets tenant B's `manager`, even when other tenants have one too. You can use the same lookup directly: + +```php +$role = KeystoneRole::findByNameForTenant('manager', $tenantId); +``` + ### Auto-Population on Creation When creating roles or permissions, the `tenant_id` is automatically populated from the authenticated user: diff --git a/src/Models/Concerns/ResolvesByNameForTenant.php b/src/Models/Concerns/ResolvesByNameForTenant.php new file mode 100644 index 0000000..21141db --- /dev/null +++ b/src/Models/Concerns/ResolvesByNameForTenant.php @@ -0,0 +1,41 @@ +withoutGlobalScope('tenant')->where('name', $name); + + if (config('keystone.features.multi_tenant', false)) { + if ($tenantId === null) { + $query->whereNull('tenant_id'); + } else { + $query->where(fn ($q) => $q->where('tenant_id', $tenantId)->orWhereNull('tenant_id')) + ->orderByRaw('tenant_id IS NULL'); + } + } + + return $query->orderBy('id')->firstOrFail(); + } + + /** + * This model's tenant, used as the reference when resolving names for it. + */ + protected function keystoneTenantId(): ?string + { + return config('keystone.features.multi_tenant', false) ? $this->tenant_id : null; + } +} diff --git a/src/Models/KeystonePermission.php b/src/Models/KeystonePermission.php index 97ace21..64e4748 100644 --- a/src/Models/KeystonePermission.php +++ b/src/Models/KeystonePermission.php @@ -2,6 +2,7 @@ namespace BSPDX\Keystone\Models; +use BSPDX\Keystone\Models\Concerns\ResolvesByNameForTenant; use BSPDX\Keystone\Services\PermissionRegistrar; use Carbon\Carbon; use Illuminate\Database\Eloquent\Builder; @@ -31,6 +32,8 @@ */ class KeystonePermission extends Model { + use ResolvesByNameForTenant; + /** * The table associated with the model. * @@ -226,6 +229,7 @@ public function getRoleNames(): Collection /** * Convert various role representations to KeystoneRole models. + * Names are resolved in this permission's tenant. */ protected function convertToRoleModels(array $roles): Collection { @@ -235,7 +239,7 @@ protected function convertToRoleModels(array $roles): Collection } if (is_string($role)) { - return KeystoneRole::where('name', $role)->firstOrFail(); + return KeystoneRole::findByNameForTenant($role, $this->keystoneTenantId()); } if (is_int($role)) { diff --git a/src/Models/KeystoneRole.php b/src/Models/KeystoneRole.php index c4356fb..131262a 100644 --- a/src/Models/KeystoneRole.php +++ b/src/Models/KeystoneRole.php @@ -3,6 +3,7 @@ namespace BSPDX\Keystone\Models; use App\Models\User; +use BSPDX\Keystone\Models\Concerns\ResolvesByNameForTenant; use Illuminate\Database\Eloquent\Builder; use Illuminate\Database\Eloquent\Model; use Illuminate\Database\Eloquent\Relations\BelongsToMany; @@ -22,6 +23,8 @@ */ class KeystoneRole extends Model { + use ResolvesByNameForTenant; + /** * The table associated with the model. */ @@ -126,13 +129,7 @@ public function users(): MorphToMany */ public function givePermissionTo(...$permissions): self { - $permissionModels = collect($permissions)->flatten()->map(function ($permission) { - if ($permission instanceof KeystonePermission) { - return $permission; - } - - return KeystonePermission::where('name', $permission)->firstOrFail(); - }); + $permissionModels = $this->convertToPermissionModels($permissions); $this->permissions()->syncWithoutDetaching($permissionModels->pluck('id')); $this->unsetRelation('permissions'); // Force reload of permissions relationship @@ -145,13 +142,7 @@ public function givePermissionTo(...$permissions): self */ public function syncPermissions(...$permissions): self { - $permissionModels = collect($permissions)->flatten()->map(function ($permission) { - if ($permission instanceof KeystonePermission) { - return $permission; - } - - return KeystonePermission::where('name', $permission)->firstOrFail(); - }); + $permissionModels = $this->convertToPermissionModels($permissions); $this->permissions()->sync($permissionModels->pluck('id')); @@ -163,9 +154,7 @@ public function syncPermissions(...$permissions): self */ public function revokePermissionTo($permission): self { - $permissionModel = $permission instanceof KeystonePermission - ? $permission - : KeystonePermission::where('name', $permission)->firstOrFail(); + $permissionModel = $this->convertToPermissionModels([$permission])->first(); $this->permissions()->detach($permissionModel->id); @@ -212,6 +201,16 @@ public function getDisplayNameAttribute(): string // HELPER METHODS // ============================================ + /** + * Convert permission names or models to models, resolving names in this role's tenant. + */ + protected function convertToPermissionModels(array $permissions): Collection + { + return collect($permissions)->flatten()->map(fn ($permission) => $permission instanceof KeystonePermission + ? $permission + : KeystonePermission::findByNameForTenant($permission, $this->keystoneTenantId())); + } + /** * Determine if this role is the super admin role. */ diff --git a/src/Traits/HasKeystone.php b/src/Traits/HasKeystone.php index 4f19cc2..8c29ecf 100644 --- a/src/Traits/HasKeystone.php +++ b/src/Traits/HasKeystone.php @@ -416,31 +416,31 @@ public function hasDirectPermission($permission, string $guard = 'web'): bool // ============================================ /** - * Convert mixed role input to KeystoneRole models + * Convert mixed role input to KeystoneRole models, resolving names in this user's tenant */ protected function convertToRoleModels($roles): Collection { - return collect($roles)->flatten()->map(function ($role) { - if ($role instanceof KeystoneRole) { - return $role; - } - - return KeystoneRole::where('name', $role)->firstOrFail(); - }); + return collect($roles)->flatten()->map(fn ($role) => $role instanceof KeystoneRole + ? $role + : KeystoneRole::findByNameForTenant($role, $this->keystoneUserTenantId())); } /** - * Convert mixed permission input to KeystonePermission models + * Convert mixed permission input to KeystonePermission models, resolving names in this user's tenant */ protected function convertToPermissionModels($permissions): Collection { - return collect($permissions)->flatten()->map(function ($permission) { - if ($permission instanceof KeystonePermission) { - return $permission; - } + return collect($permissions)->flatten()->map(fn ($permission) => $permission instanceof KeystonePermission + ? $permission + : KeystonePermission::findByNameForTenant($permission, $this->keystoneUserTenantId())); + } - return KeystonePermission::where('name', $permission)->firstOrFail(); - }); + /** + * The user's tenant for name resolution (null when multi-tenancy is off). + */ + protected function keystoneUserTenantId(): ?string + { + return config('keystone.features.multi_tenant', false) ? $this->tenant_id : null; } // ============================================ diff --git a/tests/Feature/UserRouteBindingTest.php b/tests/Feature/UserRouteBindingTest.php index 3f87847..bddbe44 100644 --- a/tests/Feature/UserRouteBindingTest.php +++ b/tests/Feature/UserRouteBindingTest.php @@ -6,6 +6,7 @@ use BSPDX\Keystone\Http\Controllers\RolePermissionController; use BSPDX\Keystone\Models\KeystonePermission; use BSPDX\Keystone\Models\KeystoneRole; +use Illuminate\Support\Facades\DB; use Illuminate\Support\Facades\Route; use Illuminate\Support\Facades\Schema; use Illuminate\Support\Str; @@ -119,6 +120,28 @@ public function tenantless_caller_can_reach_a_user_in_any_tenant() $this->assertTrue($target->fresh()->hasRole('editor')); } + #[Test] + public function tenantless_caller_assigns_the_target_tenants_role_by_name() + { + $this->requireTenantSchema(); + config(['keystone.features.multi_tenant' => true]); + $tenantA = (string) Str::uuid(); + $tenantB = (string) Str::uuid(); + KeystoneRole::withoutTenant()->create(['name' => 'manager', 'tenant_id' => $tenantA]); + $roleB = KeystoneRole::withoutTenant()->create(['name' => 'manager', 'tenant_id' => $tenantB]); + $caller = User::factory()->create(['tenant_id' => null]); + $target = User::factory()->create(['tenant_id' => $tenantB]); + + $this->actingAs($caller) + ->postJson("/users/{$target->id}/roles", ['roles' => ['manager']]) + ->assertOk(); + + $this->assertSame( + [$roleB->id], + DB::table('model_has_roles')->where('model_id', $target->id)->pluck('role_id')->all() + ); + } + /** * The single-tenant suite migrates without tenant_id columns. */ diff --git a/tests/Unit/Traits/TenantNameResolutionTest.php b/tests/Unit/Traits/TenantNameResolutionTest.php new file mode 100644 index 0000000..2c8c5e0 --- /dev/null +++ b/tests/Unit/Traits/TenantNameResolutionTest.php @@ -0,0 +1,180 @@ + true]); + } + + #[Test] + public function tenantless_caller_assigns_the_targets_tenant_role() + { + $this->role('manager', self::TENANT_A); + $roleB = $this->role('manager', self::TENANT_B); + $user = $this->user(self::TENANT_B); + + $user->assignRole('manager'); + + $this->assertSame([$roleB->id], $this->heldRoleIds($user)); + } + + #[Test] + public function tenant_role_is_preferred_over_global_role() + { + $this->role('manager', null); + $roleB = $this->role('manager', self::TENANT_B); + $user = $this->user(self::TENANT_B); + + $user->assignRole('manager'); + + $this->assertSame([$roleB->id], $this->heldRoleIds($user)); + } + + #[Test] + public function global_role_is_used_when_the_tenant_has_none() + { + $global = $this->role('auditor', null); + $user = $this->user(self::TENANT_B); + + $user->assignRole('auditor'); + + $this->assertSame([$global->id], $this->heldRoleIds($user)); + } + + #[Test] + public function name_that_exists_only_in_another_tenant_is_not_found() + { + $this->role('billing', self::TENANT_A); + $user = $this->user(self::TENANT_B); + + try { + $user->assignRole('billing'); + $this->fail('Expected ModelNotFoundException.'); + } catch (ModelNotFoundException) { + $this->assertSame([], $this->heldRoleIds($user)); + } + } + + #[Test] + public function tenantless_user_resolves_only_global_rows() + { + $this->role('manager', self::TENANT_A); + $user = $this->user(null); + + $this->expectException(ModelNotFoundException::class); + + $user->assignRole('manager'); + } + + #[Test] + public function remove_role_by_name_targets_the_targets_tenant_row() + { + $this->role('manager', self::TENANT_A); + $roleB = $this->role('manager', self::TENANT_B); + $user = $this->user(self::TENANT_B); + $user->assignRole($roleB); + + $user->removeRole('manager'); + + $this->assertSame([], $this->heldRoleIds($user)); + } + + #[Test] + public function direct_permission_by_name_resolves_in_the_users_tenant() + { + $this->permission('edit-invoices', self::TENANT_A); + $permB = $this->permission('edit-invoices', self::TENANT_B); + $user = $this->user(self::TENANT_B); + + $user->givePermissionTo('edit-invoices'); + + $this->assertSame([$permB->id], $user->permissions()->pluck('permissions.id')->all()); + } + + #[Test] + public function explicit_model_instance_is_used_as_given() + { + $roleA = $this->role('manager', self::TENANT_A); + $this->role('manager', self::TENANT_B); + $user = $this->user(self::TENANT_B); + + $user->assignRole($roleA); + + $this->assertSame([$roleA->id], $this->heldRoleIds($user)); + } + + #[Test] + public function role_permissions_by_name_resolve_in_the_roles_tenant() + { + $this->permission('edit-invoices', self::TENANT_A); + $permB = $this->permission('edit-invoices', self::TENANT_B); + $roleB = $this->role('manager', self::TENANT_B); + + $roleB->givePermissionTo('edit-invoices'); + + $this->assertSame([$permB->id], $roleB->permissions()->pluck('permissions.id')->all()); + } + + #[Test] + public function permission_roles_by_name_resolve_in_the_permissions_tenant() + { + $this->role('manager', self::TENANT_A); + $roleB = $this->role('manager', self::TENANT_B); + $permB = $this->permission('edit-invoices', self::TENANT_B); + + $permB->assignRole('manager'); + + $this->assertSame([$roleB->id], $permB->roles()->pluck('roles.id')->all()); + } + + private function role(string $name, ?string $tenantId): KeystoneRole + { + return KeystoneRole::withoutTenant()->create(['name' => $name, 'tenant_id' => $tenantId]); + } + + private function permission(string $name, ?string $tenantId): KeystonePermission + { + return KeystonePermission::withoutTenant()->create(['name' => $name, 'tenant_id' => $tenantId]); + } + + private function user(?string $tenantId): User + { + return User::factory()->create(['tenant_id' => $tenantId]); + } + + /** + * Role IDs actually stored on the user's pivot, unaffected by any scope. + * + * @return list + */ + private function heldRoleIds(User $user): array + { + return DB::table('model_has_roles') + ->where('model_type', $user->getMorphClass()) + ->where('model_id', $user->id) + ->orderBy('role_id') + ->pluck('role_id') + ->all(); + } +} From 8c6d37e97c5b2cf7f0ec1b6a8caf423d26b4122b Mon Sep 17 00:00:00 2001 From: Jason Vertucio Date: Tue, 29 Sep 2026 10:40:46 -0400 Subject: [PATCH 7/9] fix: honour app {user} bindings; settle permission cache after transactions Addresses PR #2 review (discussions r4134378963, r4134379102). - RolePermissionController user actions accept Model|string. An app-level Route::model/Route::bind('user') previously handed the controller a model that was coerced to its JSON string and 404'd on lookup. Bound users are now used as given; the cross-tenant 404 check still applies. - KeystonePermission clears the registrar cache immediately and again after the surrounding transaction commits or rolls back, so neither a concurrent refill from pre-commit rows nor an in-transaction fill of rolled-back rows survives for the cache TTL. Co-Authored-By: Claude Opus 5.5 --- .../Controllers/RolePermissionController.php | 22 +++++++----- src/Models/KeystonePermission.php | 25 +++++++++++-- tests/Feature/UserRouteBindingTest.php | 35 +++++++++++++++++++ .../Unit/Services/PermissionRegistrarTest.php | 35 +++++++++++++++++++ 4 files changed, 106 insertions(+), 11 deletions(-) diff --git a/src/Http/Controllers/RolePermissionController.php b/src/Http/Controllers/RolePermissionController.php index c21adba..c1d0046 100644 --- a/src/Http/Controllers/RolePermissionController.php +++ b/src/Http/Controllers/RolePermissionController.php @@ -112,7 +112,7 @@ public function createPermission(Request $request): JsonResponse /** * Add roles to a user, keeping the roles they already hold. */ - public function assignRoles(Request $request, string $user): JsonResponse + public function assignRoles(Request $request, Model|string $user): JsonResponse { $user = $this->resolveUser($user); @@ -136,7 +136,7 @@ public function assignRoles(Request $request, string $user): JsonResponse /** * Replace a user's roles with exactly the given set. */ - public function syncRoles(Request $request, string $user): JsonResponse + public function syncRoles(Request $request, Model|string $user): JsonResponse { $user = $this->resolveUser($user); @@ -160,7 +160,7 @@ public function syncRoles(Request $request, string $user): JsonResponse /** * Add direct permissions to a user, keeping the ones they already hold. */ - public function assignPermissions(Request $request, string $user): JsonResponse + public function assignPermissions(Request $request, Model|string $user): JsonResponse { $user = $this->resolveUser($user); @@ -184,7 +184,7 @@ public function assignPermissions(Request $request, string $user): JsonResponse /** * Replace a user's direct permissions with exactly the given set. */ - public function syncPermissions(Request $request, string $user): JsonResponse + public function syncPermissions(Request $request, Model|string $user): JsonResponse { $user = $this->resolveUser($user); @@ -244,7 +244,7 @@ public function syncRolePermissions(Request $request, KeystoneRole $role): JsonR /** * Get user's roles and permissions. */ - public function userRolesPermissions(string $user): JsonResponse + public function userRolesPermissions(Model|string $user): JsonResponse { $user = $this->resolveUser($user); @@ -300,15 +300,19 @@ public function deletePermission(KeystonePermission $permission): JsonResponse * Resolve the {user} route segment to the configured user model. * * Resolved here rather than via a global Route::bind('user') so the - * consuming app's own {user} bindings are left alone. Users in another - * tenant are reported as missing (404) rather than forbidden. + * consuming app's own {user} bindings are left alone. If the app does bind + * {user} (Route::model / Route::bind), the bound model arrives here and is + * used as-is. Users in another tenant are reported as missing (404) + * rather than forbidden, however they were resolved. */ - private function resolveUser(string $id): Model&Authenticatable + private function resolveUser(Model|string $user): Model&Authenticatable { $userModel = config('keystone.user.model') ?? config('auth.providers.users.model', User::class); - $user = $userModel::findOrFail($id); + if (! $user instanceof $userModel) { + $user = $userModel::findOrFail($user instanceof Model ? $user->getKey() : $user); + } $callerTenant = auth()->user()?->tenant_id; diff --git a/src/Models/KeystonePermission.php b/src/Models/KeystonePermission.php index 64e4748..9bb422b 100644 --- a/src/Models/KeystonePermission.php +++ b/src/Models/KeystonePermission.php @@ -92,8 +92,8 @@ protected static function booted(): void }); // Keep the registrar's cached permission list in sync with the table - static::saved(fn () => app(PermissionRegistrar::class)->forgetCachedPermissions()); - static::deleted(fn () => app(PermissionRegistrar::class)->forgetCachedPermissions()); + static::saved(fn (self $permission) => $permission->forgetCachedPermissions()); + static::deleted(fn (self $permission) => $permission->forgetCachedPermissions()); // Auto-set tenant_id and guard_name when creating permissions static::creating(function ($permission) { @@ -250,6 +250,27 @@ protected function convertToRoleModels(array $roles): Collection }); } + /** + * Invalidate the registrar's cached permission list. + * + * Cleared now so reads later in the same transaction see this change, and + * again once the transaction settles: on commit, in case another request + * refilled the cache from pre-commit rows; on rollback, in case this + * transaction refilled it with rows that no longer exist. + */ + protected function forgetCachedPermissions(): void + { + $forget = fn () => app(PermissionRegistrar::class)->forgetCachedPermissions(); + + $forget(); + + $connection = $this->getConnection(); + if ($connection->transactionLevel() > 0) { + $connection->afterCommit($forget); + $connection->afterRollBack($forget); + } + } + /** * Check if this is a global permission (accessible across all tenants). */ diff --git a/tests/Feature/UserRouteBindingTest.php b/tests/Feature/UserRouteBindingTest.php index bddbe44..b1c90cb 100644 --- a/tests/Feature/UserRouteBindingTest.php +++ b/tests/Feature/UserRouteBindingTest.php @@ -86,6 +86,41 @@ public function unknown_user_returns_404() $this->assertFalse($caller->fresh()->hasRole('editor')); } + #[Test] + public function an_app_level_user_route_binding_is_honoured() + { + Route::model('user', User::class); + [$caller, $target] = User::factory()->count(2)->create(); + + $this->actingAs($caller) + ->postJson("/users/{$target->id}/roles", ['roles' => ['editor']]) + ->assertOk() + ->assertJsonPath('user.id', $target->id); + + $this->actingAs($caller) + ->getJson("/users/{$target->id}/roles-permissions") + ->assertOk() + ->assertJsonPath('user.id', $target->id); + + $this->assertTrue($target->fresh()->hasRole('editor')); + } + + #[Test] + public function an_app_level_user_route_binding_cannot_bypass_tenant_isolation() + { + $this->requireTenantSchema(); + config(['keystone.features.multi_tenant' => true]); + Route::model('user', User::class); + $caller = User::factory()->create(['tenant_id' => (string) Str::uuid()]); + $target = User::factory()->create(['tenant_id' => (string) Str::uuid()]); + + $this->actingAs($caller) + ->postJson("/users/{$target->id}/roles", ['roles' => ['editor']]) + ->assertNotFound(); + + $this->assertFalse($target->fresh()->hasRole('editor')); + } + #[Test] public function tenant_caller_cannot_reach_a_user_in_another_tenant() { diff --git a/tests/Unit/Services/PermissionRegistrarTest.php b/tests/Unit/Services/PermissionRegistrarTest.php index 8d3018c..742cbd4 100644 --- a/tests/Unit/Services/PermissionRegistrarTest.php +++ b/tests/Unit/Services/PermissionRegistrarTest.php @@ -7,8 +7,10 @@ use BSPDX\Keystone\Services\PermissionRegistrar; use Illuminate\Contracts\Cache\Repository as CacheRepository; use Illuminate\Support\Collection; +use Illuminate\Support\Facades\DB; use Mockery; use PHPUnit\Framework\Attributes\Test; +use RuntimeException; use Tests\TestCase; class PermissionRegistrarTest extends TestCase @@ -111,4 +113,37 @@ public function permission_created_through_the_service_is_visible_immediately(): $this->assertTrue($registrar->permissionExists('archive-posts')); } + + #[Test] + public function cache_refilled_with_old_rows_before_commit_is_invalidated_on_commit(): void + { + $registrar = app(PermissionRegistrar::class); + + DB::transaction(function () { + KeystonePermission::create(['name' => 'publish-posts']); + + // Another request, not seeing the uncommitted row, refills the cache + cache()->put('keystone.permissions.all.v2', [], 3600); + }); + + $this->assertTrue($registrar->permissionExists('publish-posts')); + } + + #[Test] + public function cache_filled_inside_a_rolled_back_transaction_is_invalidated(): void + { + $registrar = app(PermissionRegistrar::class); + + try { + DB::transaction(function () use ($registrar) { + KeystonePermission::create(['name' => 'ghost-permission']); + $this->assertTrue($registrar->permissionExists('ghost-permission')); // fills the cache + + throw new RuntimeException('roll back'); + }); + } catch (RuntimeException) { + } + + $this->assertFalse($registrar->permissionExists('ghost-permission')); + } } From fe8ceebda56a4a98dd1d88f181cb50f3aa274e6c Mon Sep 17 00:00:00 2001 From: Jason Vertucio Date: Tue, 29 Sep 2026 10:51:57 -0400 Subject: [PATCH 8/9] v0.11.0 --- CHANGELOG.md | 92 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 92 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index ef37585..6dc9f2c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,98 @@ --- +## [0.11.0] - 2027-09-29 + +### Added + +- `KeystoneRole::findByNameForTenant($name, $tenantId)` and `KeystonePermission::findByNameForTenant($name, $tenantId)` look up a role or permission by name as seen from a given tenant. They ignore the caller's scope, and prefer the tenant's own row over a global one. +- `PUT /api/users/{user}/roles` (`api.users.roles.sync`), `PUT /api/users/{user}/permissions` (`api.users.permissions.sync`), and `PUT /api/roles/{role}/permissions` (`api.roles.permissions.sync`). Each **replaces** the set; send `[]` to clear it. They use the same `permission:` middleware as their `POST` counterparts. +- `AuthorizationServiceInterface::syncRolesForUser()` and `syncPermissionsForUser()` replace a user's roles or direct permissions. + +### Changed + +- **BREAKING:** In multi-tenant mode, role and permission **names** now resolve in the tenant of whatever receives them: the target user for `assignRole` / `removeRole` / `syncRoles` / `givePermissionTo` / `revokePermissionTo` / `syncPermissions` (including through the services, API and commands), the role for `KeystoneRole` permission methods, and the permission for `KeystonePermission` role methods. The receiver's own tenant row wins over a global row with the same name. A receiver with no tenant only matches global rows. A name that exists only in another tenant now throws `ModelNotFoundException` (404 through the API) instead of attaching that tenant's row. Single-tenant installs are unaffected. +- **BREAKING:** `hasRole()`, `hasAnyRole()`, and `hasAllRoles()` on `HasKeystone` no longer pass automatically for super-admins. They now return `true` only for roles the user actually holds. Before, a super-admin "had" every role, including role names that don't exist. The super-admin bypass is unchanged everywhere access is enforced: permission checks, `can()` / `Gate`, the `role:` and `permission:` middleware, and `AuthorizationService::userHasRole()` / `userHasAnyRole()` / `userHasAllRoles()`. +- **BREAKING:** `POST /api/users/{user}/roles`, `POST /api/users/{user}/permissions`, and `POST /api/roles/{role}/permissions` now **add** to the existing set. Before, they replaced it. Already-held items are not duplicated, and an empty array is rejected with 422. +- **BREAKING:** `AuthorizationServiceInterface::assignRolesToUser()` / `assignPermissionsToUser()` now **add** instead of replacing. +- **BREAKING (custom implementers only):** `AuthorizationServiceInterface` has two new methods, `syncRolesForUser()` and `syncPermissionsForUser()`. +- **BREAKING:** `KeystonePermission::forTenant($tenantId)` now returns only that tenant's permissions. It no longer includes global permissions (`tenant_id = NULL`), and it no longer removes the automatic tenant scope. This makes it match `KeystoneRole::forTenant()`, which already behaved this way. Both scopes now also qualify `tenant_id` with the table name, to avoid ambiguous-column errors in joined queries. +- The permission-list cache key is now `keystone.permissions.all.v2`, and the cache stores plain `name`/`guard_name` arrays instead of serialized Eloquent models. Entries under the old `keystone.permissions.all` key are ignored and expire on their own. + +### Fixed + +- Assigning roles or permissions **by name** in multi-tenant mode could attach **another tenant's** row with the same name. Names were resolved in the logged-in caller's tenant scope, so a caller with no tenant (a global admin, a console command, or a queued job) matched rows from every tenant and took whichever came first. A name existing both in the target's tenant and globally also gave an arbitrary result. Reported in PR #2 review. +- A super-admin no longer "has" nonexistent roles: `hasRole('typo-role')` returned `true`, which hid typos and gave wrong identity answers (badges, dashboards, user lists). +- The `keystone.permissions.all.v2` cache is now cleared automatically whenever a `KeystonePermission` is created, updated, or deleted, whether through the model, `PermissionService`, the API, the console commands, or the seeder. Before this, only the console commands cleared it, so `PermissionRegistrar::permissionExists()` / `getAllPermissionNames()` could return a stale list for up to `rbac.cache_expiration` (24 hours by default). Bulk query updates and deletes (`KeystonePermission::query()->update()/delete()`) bypass model events. Call `CacheServiceInterface::clearPermissionCache()` after those. +- The `role:` and `permission:` middleware no longer call `redirect()->route('login')` for unauthenticated requests. That route isn't defined by Keystone, so apps without a route named `login` got a 500 `RouteNotFoundException`. They now throw `Illuminate\Auth\AuthenticationException`, the same as Laravel's `auth` middleware, and the app's own guest handling decides the response. JSON requests get **401**. Browser requests are redirected via the app's `redirectGuestsTo` / `AuthenticationException::redirectUsing()` callback, or to the `login` route if one exists, and the intended URL is now remembered. Web apps with a `login` route see the same redirect as before. +- `/users/{user}/...` API endpoints (`assignRoles`, `syncRoles`, `assignPermissions`, `syncPermissions`, `userRolesPermissions`) returned 404 for every user when the consuming application defined its own `{user}` route binding (`Route::model('user', ...)` or `Route::bind('user', ...)`). The bound model was converted to its JSON string and then looked up as a user ID. The controller now uses the bound user model as it is, and still applies the cross-tenant 404 check to it. +- The permission list cache (`PermissionRegistrar::permissionExists()` / `getAllPermissionNames()`) could keep stale data for the full cache TTL when permissions changed inside a database transaction. It is now also cleared after the transaction commits or rolls back. Before, a concurrent request could refill it from the pre-commit rows, or the transaction itself could fill it with rows that were then rolled back. + +### Removed + +- The no-op `KeystonePermission::forgetCachedPermissions()` (an unimplemented TODO), and the `HasKeystone` code that cleared an unused `user_permissions_{id}` cache key after every role/permission change. Both methods were `protected` and had no observable effect. If your User model overrode `forgetCachedPermissions()`, Keystone no longer calls it. + +### Security + +- **Management API `/api/users/{user}/…` routes acted on the authenticated caller instead of the user in the URL.** `RolePermissionController` type-hinted the `{user}` parameter as the `Authenticatable` interface, which route model binding can't resolve. Laravel filled it with the logged-in user instead. Any caller holding `assign-roles` could therefore grant **themselves** any role, including `super-admin`, via `POST /api/users/{anyone}/roles`. `POST /api/users/{user}/permissions` had the same flaw, and `GET /api/users/{user}/roles-permissions` always returned the caller's own data. The controller now resolves `{user}` against the configured user model (`keystone.user.model`, else `auth.providers.users.model`) and returns **404** for unknown IDs. In multi-tenant mode, a caller who has a tenant gets **404** for users in other tenants. Keystone does not register a global `{user}` route binding, so your app's own bindings are untouched. Non-breaking. **If you registered the example API routes, audit role and permission assignments for accounts that granted themselves access**, for example `super-admin` held by users who only had `assign-roles`. + +### Testing + +- Added commit and rollback coverage for permission cache invalidation in `tests/Unit/Services/PermissionRegistrarTest.php`. +- Added coverage to `tests/Feature/UserRouteBindingTest.php` for app-level `{user}` bindings, including a check that a tenant caller still gets a 404 for a user in another tenant when the app binds `{user}`. +- All 106 tests passing (259 assertions); single-tenant suite passing (38 tests, 4 skipped). + +### Breaking Changes + +#### `KeystonePermission::forTenant()` excludes globals and respects the tenant scope + +`forTenant()` now means "this tenant's rows only" on both models. A tenant user calling `forTenant()` with another tenant's ID now gets an empty result instead of that tenant's permissions. + +**Migration Guide:** + +1. Search your code for `KeystonePermission::forTenant(` (and `->forTenant(` on permission queries). +2. If you wanted **the current user's tenant plus global permissions**, drop `forTenant()`. The automatic tenant scope already gives you exactly that: `KeystonePermission::query()->get()`. +3. If you wanted **a specific tenant plus global permissions**, write it explicitly: + ```php + KeystonePermission::withoutTenant() + ->where(fn ($q) => $q->where('tenant_id', $tenantId)->orWhereNull('tenant_id')) + ->get(); + ``` +4. If you relied on `forTenant()` to **read another tenant's permissions**, add `withoutTenant()` first: `KeystonePermission::withoutTenant()->forTenant($tenantId)->get()`. + +#### "Assign" now adds; use `PUT` / `sync*ForUser()` to replace + +The assign endpoints and service methods used to wipe and replace the whole set. They now keep existing assignments. + +**Migration Guide:** + +1. **Published route files:** if you published `routes/keystone-api.php`, copy the three new `PUT` routes (`api.users.roles.sync`, `api.users.permissions.sync`, `api.roles.permissions.sync`) from the package's `routes/api.php`. Your existing `POST` routes keep working, but they now add. +2. **API clients** that send the full desired set with `POST` and expect everything else to be removed must switch to `PUT` with the same body. +3. **Service callers:** replace `assignRolesToUser()` / `assignPermissionsToUser()` with `syncRolesForUser()` / `syncPermissionsForUser()` wherever you relied on replacement. +4. **Custom `AuthorizationServiceInterface` implementations** must add `syncRolesForUser()` and `syncPermissionsForUser()`. + +#### Role checks are literal for super-admins + +**Migration Guide:** + +1. Search your code for `->hasRole(`, `->hasAnyRole(`, and `->hasAllRoles(`, including Blade `@if` checks and policies. +2. If a check is about **identity** ("is this user an editor?"), leave it. It's now correct. +3. If a check is about **access** and super-admins should pass, switch to one of these: + - `$user->isSuperAdmin() || $user->hasRole('editor')` + - `app(AuthorizationServiceInterface::class)->userHasRole($user, 'editor')` (keeps the bypass) + - a permission check (`$user->can('edit-posts')` / `hasPermissionTo()`), which is the recommended way to gate access + - assigning the role to your super-admins explicitly +4. The `role:` middleware is unaffected. Super-admins still pass role-gated routes. + +#### Names resolve in the receiver's tenant + +**Migration Guide:** + +1. Most apps need no changes. Assignments within a tenant, and assignments of global roles and permissions, behave as before and are now deterministic. +2. If you give a user **with no tenant** a **tenant-specific** role or permission by name, pass the model instead: `$user->assignRole(KeystoneRole::withoutTenant()->forTenant($tenantId)->where('name', 'manager')->firstOrFail())`. Model instances are always used as given. +3. If you relied on a global admin, command, or job attaching a role from a *different* tenant by name, that was the bug. Pass the model explicitly if it's really intended. +4. Code that catches `ModelNotFoundException` from assignment methods may now see it where a wrong-tenant row used to be attached silently. + ## [0.10.1] - 2026-08-17 ### Fixed From 602b1b4b8bef6ee9ee9f57fbc2b0f1eacccf4a38 Mon Sep 17 00:00:00 2001 From: Jason Vertucio Date: Tue, 29 Sep 2026 10:53:06 -0400 Subject: [PATCH 9/9] chore: Add changelog --- CHANGELOG.md | 92 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 92 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index ef37585..6dc9f2c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,98 @@ --- +## [0.11.0] - 2027-09-29 + +### Added + +- `KeystoneRole::findByNameForTenant($name, $tenantId)` and `KeystonePermission::findByNameForTenant($name, $tenantId)` look up a role or permission by name as seen from a given tenant. They ignore the caller's scope, and prefer the tenant's own row over a global one. +- `PUT /api/users/{user}/roles` (`api.users.roles.sync`), `PUT /api/users/{user}/permissions` (`api.users.permissions.sync`), and `PUT /api/roles/{role}/permissions` (`api.roles.permissions.sync`). Each **replaces** the set; send `[]` to clear it. They use the same `permission:` middleware as their `POST` counterparts. +- `AuthorizationServiceInterface::syncRolesForUser()` and `syncPermissionsForUser()` replace a user's roles or direct permissions. + +### Changed + +- **BREAKING:** In multi-tenant mode, role and permission **names** now resolve in the tenant of whatever receives them: the target user for `assignRole` / `removeRole` / `syncRoles` / `givePermissionTo` / `revokePermissionTo` / `syncPermissions` (including through the services, API and commands), the role for `KeystoneRole` permission methods, and the permission for `KeystonePermission` role methods. The receiver's own tenant row wins over a global row with the same name. A receiver with no tenant only matches global rows. A name that exists only in another tenant now throws `ModelNotFoundException` (404 through the API) instead of attaching that tenant's row. Single-tenant installs are unaffected. +- **BREAKING:** `hasRole()`, `hasAnyRole()`, and `hasAllRoles()` on `HasKeystone` no longer pass automatically for super-admins. They now return `true` only for roles the user actually holds. Before, a super-admin "had" every role, including role names that don't exist. The super-admin bypass is unchanged everywhere access is enforced: permission checks, `can()` / `Gate`, the `role:` and `permission:` middleware, and `AuthorizationService::userHasRole()` / `userHasAnyRole()` / `userHasAllRoles()`. +- **BREAKING:** `POST /api/users/{user}/roles`, `POST /api/users/{user}/permissions`, and `POST /api/roles/{role}/permissions` now **add** to the existing set. Before, they replaced it. Already-held items are not duplicated, and an empty array is rejected with 422. +- **BREAKING:** `AuthorizationServiceInterface::assignRolesToUser()` / `assignPermissionsToUser()` now **add** instead of replacing. +- **BREAKING (custom implementers only):** `AuthorizationServiceInterface` has two new methods, `syncRolesForUser()` and `syncPermissionsForUser()`. +- **BREAKING:** `KeystonePermission::forTenant($tenantId)` now returns only that tenant's permissions. It no longer includes global permissions (`tenant_id = NULL`), and it no longer removes the automatic tenant scope. This makes it match `KeystoneRole::forTenant()`, which already behaved this way. Both scopes now also qualify `tenant_id` with the table name, to avoid ambiguous-column errors in joined queries. +- The permission-list cache key is now `keystone.permissions.all.v2`, and the cache stores plain `name`/`guard_name` arrays instead of serialized Eloquent models. Entries under the old `keystone.permissions.all` key are ignored and expire on their own. + +### Fixed + +- Assigning roles or permissions **by name** in multi-tenant mode could attach **another tenant's** row with the same name. Names were resolved in the logged-in caller's tenant scope, so a caller with no tenant (a global admin, a console command, or a queued job) matched rows from every tenant and took whichever came first. A name existing both in the target's tenant and globally also gave an arbitrary result. Reported in PR #2 review. +- A super-admin no longer "has" nonexistent roles: `hasRole('typo-role')` returned `true`, which hid typos and gave wrong identity answers (badges, dashboards, user lists). +- The `keystone.permissions.all.v2` cache is now cleared automatically whenever a `KeystonePermission` is created, updated, or deleted, whether through the model, `PermissionService`, the API, the console commands, or the seeder. Before this, only the console commands cleared it, so `PermissionRegistrar::permissionExists()` / `getAllPermissionNames()` could return a stale list for up to `rbac.cache_expiration` (24 hours by default). Bulk query updates and deletes (`KeystonePermission::query()->update()/delete()`) bypass model events. Call `CacheServiceInterface::clearPermissionCache()` after those. +- The `role:` and `permission:` middleware no longer call `redirect()->route('login')` for unauthenticated requests. That route isn't defined by Keystone, so apps without a route named `login` got a 500 `RouteNotFoundException`. They now throw `Illuminate\Auth\AuthenticationException`, the same as Laravel's `auth` middleware, and the app's own guest handling decides the response. JSON requests get **401**. Browser requests are redirected via the app's `redirectGuestsTo` / `AuthenticationException::redirectUsing()` callback, or to the `login` route if one exists, and the intended URL is now remembered. Web apps with a `login` route see the same redirect as before. +- `/users/{user}/...` API endpoints (`assignRoles`, `syncRoles`, `assignPermissions`, `syncPermissions`, `userRolesPermissions`) returned 404 for every user when the consuming application defined its own `{user}` route binding (`Route::model('user', ...)` or `Route::bind('user', ...)`). The bound model was converted to its JSON string and then looked up as a user ID. The controller now uses the bound user model as it is, and still applies the cross-tenant 404 check to it. +- The permission list cache (`PermissionRegistrar::permissionExists()` / `getAllPermissionNames()`) could keep stale data for the full cache TTL when permissions changed inside a database transaction. It is now also cleared after the transaction commits or rolls back. Before, a concurrent request could refill it from the pre-commit rows, or the transaction itself could fill it with rows that were then rolled back. + +### Removed + +- The no-op `KeystonePermission::forgetCachedPermissions()` (an unimplemented TODO), and the `HasKeystone` code that cleared an unused `user_permissions_{id}` cache key after every role/permission change. Both methods were `protected` and had no observable effect. If your User model overrode `forgetCachedPermissions()`, Keystone no longer calls it. + +### Security + +- **Management API `/api/users/{user}/…` routes acted on the authenticated caller instead of the user in the URL.** `RolePermissionController` type-hinted the `{user}` parameter as the `Authenticatable` interface, which route model binding can't resolve. Laravel filled it with the logged-in user instead. Any caller holding `assign-roles` could therefore grant **themselves** any role, including `super-admin`, via `POST /api/users/{anyone}/roles`. `POST /api/users/{user}/permissions` had the same flaw, and `GET /api/users/{user}/roles-permissions` always returned the caller's own data. The controller now resolves `{user}` against the configured user model (`keystone.user.model`, else `auth.providers.users.model`) and returns **404** for unknown IDs. In multi-tenant mode, a caller who has a tenant gets **404** for users in other tenants. Keystone does not register a global `{user}` route binding, so your app's own bindings are untouched. Non-breaking. **If you registered the example API routes, audit role and permission assignments for accounts that granted themselves access**, for example `super-admin` held by users who only had `assign-roles`. + +### Testing + +- Added commit and rollback coverage for permission cache invalidation in `tests/Unit/Services/PermissionRegistrarTest.php`. +- Added coverage to `tests/Feature/UserRouteBindingTest.php` for app-level `{user}` bindings, including a check that a tenant caller still gets a 404 for a user in another tenant when the app binds `{user}`. +- All 106 tests passing (259 assertions); single-tenant suite passing (38 tests, 4 skipped). + +### Breaking Changes + +#### `KeystonePermission::forTenant()` excludes globals and respects the tenant scope + +`forTenant()` now means "this tenant's rows only" on both models. A tenant user calling `forTenant()` with another tenant's ID now gets an empty result instead of that tenant's permissions. + +**Migration Guide:** + +1. Search your code for `KeystonePermission::forTenant(` (and `->forTenant(` on permission queries). +2. If you wanted **the current user's tenant plus global permissions**, drop `forTenant()`. The automatic tenant scope already gives you exactly that: `KeystonePermission::query()->get()`. +3. If you wanted **a specific tenant plus global permissions**, write it explicitly: + ```php + KeystonePermission::withoutTenant() + ->where(fn ($q) => $q->where('tenant_id', $tenantId)->orWhereNull('tenant_id')) + ->get(); + ``` +4. If you relied on `forTenant()` to **read another tenant's permissions**, add `withoutTenant()` first: `KeystonePermission::withoutTenant()->forTenant($tenantId)->get()`. + +#### "Assign" now adds; use `PUT` / `sync*ForUser()` to replace + +The assign endpoints and service methods used to wipe and replace the whole set. They now keep existing assignments. + +**Migration Guide:** + +1. **Published route files:** if you published `routes/keystone-api.php`, copy the three new `PUT` routes (`api.users.roles.sync`, `api.users.permissions.sync`, `api.roles.permissions.sync`) from the package's `routes/api.php`. Your existing `POST` routes keep working, but they now add. +2. **API clients** that send the full desired set with `POST` and expect everything else to be removed must switch to `PUT` with the same body. +3. **Service callers:** replace `assignRolesToUser()` / `assignPermissionsToUser()` with `syncRolesForUser()` / `syncPermissionsForUser()` wherever you relied on replacement. +4. **Custom `AuthorizationServiceInterface` implementations** must add `syncRolesForUser()` and `syncPermissionsForUser()`. + +#### Role checks are literal for super-admins + +**Migration Guide:** + +1. Search your code for `->hasRole(`, `->hasAnyRole(`, and `->hasAllRoles(`, including Blade `@if` checks and policies. +2. If a check is about **identity** ("is this user an editor?"), leave it. It's now correct. +3. If a check is about **access** and super-admins should pass, switch to one of these: + - `$user->isSuperAdmin() || $user->hasRole('editor')` + - `app(AuthorizationServiceInterface::class)->userHasRole($user, 'editor')` (keeps the bypass) + - a permission check (`$user->can('edit-posts')` / `hasPermissionTo()`), which is the recommended way to gate access + - assigning the role to your super-admins explicitly +4. The `role:` middleware is unaffected. Super-admins still pass role-gated routes. + +#### Names resolve in the receiver's tenant + +**Migration Guide:** + +1. Most apps need no changes. Assignments within a tenant, and assignments of global roles and permissions, behave as before and are now deterministic. +2. If you give a user **with no tenant** a **tenant-specific** role or permission by name, pass the model instead: `$user->assignRole(KeystoneRole::withoutTenant()->forTenant($tenantId)->where('name', 'manager')->firstOrFail())`. Model instances are always used as given. +3. If you relied on a global admin, command, or job attaching a role from a *different* tenant by name, that was the bug. Pass the model explicitly if it's really intended. +4. Code that catches `ModelNotFoundException` from assignment methods may now see it where a wrong-tenant row used to be attached silently. + ## [0.10.1] - 2026-08-17 ### Fixed