Conversation
…e 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 <noreply@anthropic.com>
- 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 <noreply@anthropic.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Tenantless assignment can select another tenant’s role, existing route bindings can cause type errors, and cache invalidation is not transaction-safe.
Review effort: Balanced
Findings: 3
Open (3)
What changed in this PR
Refactors permission caching, role assignment semantics, tenant isolation, and user targeting across Keystone’s RBAC APIs.
Changes:
- Adds additive POST and replacing PUT assignment operations.
- Corrects user targeting and tenant-aware role checks.
- Adds permission-cache invalidation, PHPStan tooling, documentation, and tests.
| File | Description |
|---|---|
tests/Unit/Traits/MultiTenantRoleAssignmentTest.php |
Tests tenant-scoped role visibility. |
tests/Unit/Services/PermissionRegistrarTest.php |
Tests cache key and invalidation. |
tests/Unit/Services/AuthorizationServiceTest.php |
Tests additive and replacing assignments. |
tests/Unit/Models/MultiTenantRoleTest.php |
Tests role tenant scopes. |
tests/Unit/Models/MultiTenantPermissionTest.php |
Tests permission tenant scopes. |
tests/Unit/HasKeystoneTraitTest.php |
Tests super-admin semantics. |
tests/Feature/UserRouteBindingTest.php |
Tests user targeting and tenant isolation. |
tests/Feature/SingleTenantApiTest.php |
Tests assignment without tenant columns. |
tests/Feature/RolePermissionApiTest.php |
Tests POST/PUT API behavior. |
tests/Feature/KeystoneTest.php |
Tests middleware guest handling and bypass. |
tests/Feature/AssignRoleCommandTest.php |
Tests additive and sync command modes. |
src/Traits/HasKeystone.php |
Makes role checks literal and removes obsolete caching. |
src/Services/RoleService.php |
Reloads user roles directly. |
src/Services/PermissionService.php |
Reloads direct permissions directly. |
src/Services/PermissionRegistrar.php |
Updates cached permission representation and key. |
src/Services/Contracts/AuthorizationServiceInterface.php |
Adds explicit synchronization methods. |
src/Services/AuthorizationService.php |
Implements additive and replacing assignments. |
src/Models/KeystoneRole.php |
Clarifies and qualifies tenant scopes. |
src/Models/KeystonePermission.php |
Adds permission-cache invalidation and tenant scopes. |
src/Http/Middleware/EnsureHasRole.php |
Uses standard authentication exceptions. |
src/Http/Middleware/EnsureHasPermission.php |
Uses standard authentication exceptions. |
src/Http/Controllers/RolePermissionController.php |
Adds assignment endpoints and manual user resolution. |
src/Console/Commands/UnassignRoleCommand.php |
Resolves linting issues. |
src/Console/Commands/UnassignPermissionCommand.php |
Resolves linting issues. |
src/Console/Commands/Concerns/InteractsWithKeystone.php |
Safely handles optional guard options. |
src/Console/Commands/AssignRoleCommand.php |
Uses explicit sync behavior. |
src/Console/Commands/AssignPermissionCommand.php |
Resolves linting issues. |
routes/api.php |
Adds PUT synchronization routes. |
README.md |
Documents assignment and tenant semantics. |
phpstan.neon |
Configures Larastan analysis. |
docs/multi-tenancy.md |
Documents tenant scopes and role checks. |
composer.json |
Adds Larastan and quality scripts. |
app/Models/User.php |
Documents the tenant property. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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 <noreply@anthropic.com>
…ctions 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 <noreply@anthropic.com>
jvjvjv
added a commit
that referenced
this pull request
Sep 29, 2026
* refactor: Update cache key for permissions and enhance getPermissions method
* refactor: Update cache key for permissions and enhance getPermissions method
* 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 <noreply@anthropic.com>
* feat: add PHPStan configuration and linting scripts to enhance code quality
* 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 <noreply@anthropic.com>
* 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 <noreply@anthropic.com>
* 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 <noreply@anthropic.com>
* v0.11.0
* chore: Add changelog
---------
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Update the permissions cache key and improve the
getPermissionsmethod. Fix user targeting in API routes to prevent unauthorized role assignments. Introduce new methods for additive role and permission assignments, ensuring existing roles and permissions are preserved. Enhance code quality with PHPStan configuration and resolve linting issues. Update tests to reflect changes in role assignment behavior.This pull request introduces improved clarity and flexibility for role and permission assignment in a multi-tenant Laravel application, along with enhanced documentation and developer tooling. The main changes include new API endpoints and controller methods for both additive and replacement (sync) operations on user and role assignments, expanded documentation on multi-tenancy and super-admin behavior, and the addition of static analysis tooling.
API and Controller Enhancements
PUTendpoints and controller methods (syncRoles,syncPermissions,syncRolePermissions) to replace all roles or permissions for a user or role, in addition to the existingPOSTendpoints which now only add to existing assignments. This provides clear distinction between additive and replacement operations. [1] [2] [3] [4]Documentation Improvements
README.mdanddocs/multi-tenancy.mdto clarify the difference between adding and replacing roles/permissions, document the new API endpoints and their usage, and provide details on tenant query scopes and super-admin role checks. [1] [2] [3] [4] [5] [6] [7] [8]Multi-Tenancy Support
global(),tenantSpecific(),forTenant(),withoutTenant()) onKeystoneRoleandKeystonePermissionmodels to make multi-tenant data access more explicit and flexible. [1] [2]Developer Tooling
Console Command Consistency
These changes collectively provide a more robust, clear, and developer-friendly approach to managing roles and permissions in a multi-tenant environment.