From ea92bfecb8efe2742dbfe20828f8ca2b40c7605c Mon Sep 17 00:00:00 2001 From: Holger Schmermbeck Date: Sat, 1 Nov 2025 19:58:42 +0100 Subject: [PATCH 1/3] feat: add SetTenant middleware and Spatie Permission RBAC MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Install and configure spatie/laravel-permission with teams support - Configure tenant_id as team_foreign_key for multi-tenancy - Create SetTenant middleware extracting tenant from path/header - Validate tenant existence and set PermissionRegistrar team ID - Register middleware alias in bootstrap/app.php - Add 9 comprehensive PEST tests for middleware functionality - Test tenant isolation, missing tenant, invalid tenant scenarios - Configure PHPStan ignores for vendor migration and test methods Resolves #50 (PR-3: Tenant Middleware & RBAC Wiring) Quality checks: ✅ PSR-12 compliant (Pint) ✅ PHPStan level max (0 errors) ✅ 39 tests passing (89 assertions) ✅ 593 LOC (under 600 limit) --- app/Http/Middleware/SetTenant.php | 79 +++++++ bootstrap/app.php | 4 +- config/permission.php | 208 ++++++++++++++++++ ..._11_01_185152_create_permission_tables.php | 140 ++++++++++++ phpstan.neon | 15 ++ tests/Feature/SetTenantMiddlewareTest.php | 148 +++++++++++++ 6 files changed, 593 insertions(+), 1 deletion(-) create mode 100644 app/Http/Middleware/SetTenant.php create mode 100644 config/permission.php create mode 100644 database/migrations/2025_11_01_185152_create_permission_tables.php create mode 100644 tests/Feature/SetTenantMiddlewareTest.php diff --git a/app/Http/Middleware/SetTenant.php b/app/Http/Middleware/SetTenant.php new file mode 100644 index 0000000..fb82a46 --- /dev/null +++ b/app/Http/Middleware/SetTenant.php @@ -0,0 +1,79 @@ +extractTenantId($request); + + if ($tenantId === null) { + return response()->json([ + 'error' => 'Tenant ID is required', + 'message' => 'Please provide tenant ID in path (/tenants/{tenant}) or X-Tenant header', + ], 400); + } + + // Verify tenant exists + if (! TenantKey::where('id', $tenantId)->exists()) { + return response()->json([ + 'error' => 'Tenant not found', + 'message' => 'The specified tenant does not exist', + ], 404); + } + + // Set tenant for Spatie Permission (team-based permissions) + app(PermissionRegistrar::class)->setPermissionsTeamId($tenantId); + + // Store tenant ID in request for use in controllers + $request->merge(['tenant_id' => $tenantId]); + + return $next($request); + } + + /** + * Extract tenant ID from request path or header. + * + * Priority: path parameter > X-Tenant header + */ + private function extractTenantId(Request $request): ?int + { + // Try to get from route parameter first + $tenantId = $request->route('tenant'); + + if ($tenantId !== null) { + return (int) $tenantId; + } + + // Fallback to X-Tenant header + $headerTenant = $request->header('X-Tenant'); + + if ($headerTenant !== null) { + return (int) $headerTenant; + } + + return null; + } +} diff --git a/bootstrap/app.php b/bootstrap/app.php index 35732ae..237de01 100644 --- a/bootstrap/app.php +++ b/bootstrap/app.php @@ -14,7 +14,9 @@ health: '/up', ) ->withMiddleware(function (Middleware $middleware): void { - // + $middleware->alias([ + 'tenant' => \App\Http\Middleware\SetTenant::class, + ]); }) ->withExceptions(function (Exceptions $exceptions): void { // diff --git a/config/permission.php b/config/permission.php new file mode 100644 index 0000000..c15eeba --- /dev/null +++ b/config/permission.php @@ -0,0 +1,208 @@ + [ + + /* + * When using the "HasPermissions" trait from this package, we need to know which + * Eloquent model should be used to retrieve your permissions. Of course, it + * is often just the "Permission" model but you may use whatever you like. + * + * The model you want to use as a Permission model needs to implement the + * `Spatie\Permission\Contracts\Permission` contract. + */ + + 'permission' => Spatie\Permission\Models\Permission::class, + + /* + * When using the "HasRoles" trait from this package, we need to know which + * Eloquent model should be used to retrieve your roles. Of course, it + * is often just the "Role" model but you may use whatever you like. + * + * The model you want to use as a Role model needs to implement the + * `Spatie\Permission\Contracts\Role` contract. + */ + + 'role' => Spatie\Permission\Models\Role::class, + + ], + + 'table_names' => [ + + /* + * When using the "HasRoles" trait from this package, we need to know which + * table should be used to retrieve your roles. We have chosen a basic + * default value but you may easily change it to any table you like. + */ + + 'roles' => 'roles', + + /* + * When using the "HasPermissions" trait from this package, we need to know which + * table should be used to retrieve your permissions. We have chosen a basic + * default value but you may easily change it to any table you like. + */ + + 'permissions' => 'permissions', + + /* + * When using the "HasPermissions" trait from this package, we need to know which + * table should be used to retrieve your models permissions. We have chosen a + * basic default value but you may easily change it to any table you like. + */ + + 'model_has_permissions' => 'model_has_permissions', + + /* + * When using the "HasRoles" trait from this package, we need to know which + * table should be used to retrieve your models roles. We have chosen a + * basic default value but you may easily change it to any table you like. + */ + + 'model_has_roles' => 'model_has_roles', + + /* + * When using the "HasRoles" trait from this package, we need to know which + * table should be used to retrieve your roles permissions. We have chosen a + * basic default value but you may easily change it to any table you like. + */ + + 'role_has_permissions' => 'role_has_permissions', + ], + + 'column_names' => [ + /* + * Change this if you want to name the related pivots other than defaults + */ + 'role_pivot_key' => null, // default 'role_id', + 'permission_pivot_key' => null, // default 'permission_id', + + /* + * Change this if you want to name the related model primary key other than + * `model_id`. + * + * For example, this would be nice if your primary keys are all UUIDs. In + * that case, name this `model_uuid`. + */ + + 'model_morph_key' => 'model_id', + + /* + * Change this if you want to use the teams feature and your related model's + * foreign key is other than `team_id`. + */ + + 'team_foreign_key' => 'tenant_id', + ], + + /* + * When set to true, the method for checking permissions will be registered on the gate. + * Set this to false if you want to implement custom logic for checking permissions. + */ + + 'register_permission_check_method' => true, + + /* + * When set to true, Laravel\Octane\Events\OperationTerminated event listener will be registered + * this will refresh permissions on every TickTerminated, TaskTerminated and RequestTerminated + * NOTE: This should not be needed in most cases, but an Octane/Vapor combination benefited from it. + */ + 'register_octane_reset_listener' => false, + + /* + * Events will fire when a role or permission is assigned/unassigned: + * \Spatie\Permission\Events\RoleAttached + * \Spatie\Permission\Events\RoleDetached + * \Spatie\Permission\Events\PermissionAttached + * \Spatie\Permission\Events\PermissionDetached + * + * To enable, set to true, and then create listeners to watch these events. + */ + 'events_enabled' => false, + + /* + * Teams Feature. + * When set to true the package implements teams using the 'team_foreign_key'. + * If you want the migrations to register the 'team_foreign_key', you must + * set this to true before doing the migration. + * If you already did the migration then you must make a new migration to also + * add 'team_foreign_key' to 'roles', 'model_has_roles', and 'model_has_permissions' + * (view the latest version of this package's migration file) + */ + + 'teams' => true, + + /* + * The class to use to resolve the permissions team id + */ + 'team_resolver' => \Spatie\Permission\DefaultTeamResolver::class, + + /* + * Passport Client Credentials Grant + * When set to true the package will use Passports Client to check permissions + */ + + 'use_passport_client_credentials' => false, + + /* + * When set to true, the required permission names are added to exception messages. + * This could be considered an information leak in some contexts, so the default + * setting is false here for optimum safety. + */ + + 'display_permission_in_exception' => false, + + /* + * When set to true, the required role names are added to exception messages. + * This could be considered an information leak in some contexts, so the default + * setting is false here for optimum safety. + */ + + 'display_role_in_exception' => false, + + /* + * By default wildcard permission lookups are disabled. + * See documentation to understand supported syntax. + */ + + 'enable_wildcard_permission' => false, + + /* + * The class to use for interpreting wildcard permissions. + * If you need to modify delimiters, override the class and specify its name here. + */ + // 'wildcard_permission' => Spatie\Permission\WildcardPermission::class, + + /* Cache-specific settings */ + + 'cache' => [ + + /* + * By default all permissions are cached for 24 hours to speed up performance. + * When permissions or roles are updated the cache is flushed automatically. + */ + + 'expiration_time' => \DateInterval::createFromDateString('24 hours'), + + /* + * The cache key used to store all permissions. + */ + + 'key' => 'spatie.permission.cache', + + /* + * You may optionally indicate a specific cache driver to use for permission and + * role caching using any of the `store` drivers listed in the cache.php config + * file. Using 'default' here means to use the `default` set in cache.php. + */ + + 'store' => 'default', + ], +]; diff --git a/database/migrations/2025_11_01_185152_create_permission_tables.php b/database/migrations/2025_11_01_185152_create_permission_tables.php new file mode 100644 index 0000000..d94525e --- /dev/null +++ b/database/migrations/2025_11_01_185152_create_permission_tables.php @@ -0,0 +1,140 @@ +engine('InnoDB'); + $table->bigIncrements('id'); // permission id + $table->string('name'); // For MyISAM use string('name', 225); // (or 166 for InnoDB with Redundant/Compact row format) + $table->string('guard_name'); // For MyISAM use string('guard_name', 25); + $table->timestamps(); + + $table->unique(['name', 'guard_name']); + }); + + Schema::create($tableNames['roles'], static function (Blueprint $table) use ($teams, $columnNames) { + // $table->engine('InnoDB'); + $table->bigIncrements('id'); // role id + if ($teams || config('permission.testing')) { // permission.testing is a fix for sqlite testing + $table->unsignedBigInteger($columnNames['team_foreign_key'])->nullable(); + $table->index($columnNames['team_foreign_key'], 'roles_team_foreign_key_index'); + } + $table->string('name'); // For MyISAM use string('name', 225); // (or 166 for InnoDB with Redundant/Compact row format) + $table->string('guard_name'); // For MyISAM use string('guard_name', 25); + $table->timestamps(); + if ($teams || config('permission.testing')) { + $table->unique([$columnNames['team_foreign_key'], 'name', 'guard_name']); + } else { + $table->unique(['name', 'guard_name']); + } + }); + + Schema::create($tableNames['model_has_permissions'], static function (Blueprint $table) use ($tableNames, $columnNames, $pivotPermission, $teams) { + $table->unsignedBigInteger($pivotPermission); + + $table->string('model_type'); + $table->unsignedBigInteger($columnNames['model_morph_key']); + $table->index([$columnNames['model_morph_key'], 'model_type'], 'model_has_permissions_model_id_model_type_index'); + + $table->foreign($pivotPermission) + ->references('id') // permission id + ->on($tableNames['permissions']) + ->onDelete('cascade'); + if ($teams) { + $table->unsignedBigInteger($columnNames['team_foreign_key']); + $table->index($columnNames['team_foreign_key'], 'model_has_permissions_team_foreign_key_index'); + + $table->primary([$columnNames['team_foreign_key'], $pivotPermission, $columnNames['model_morph_key'], 'model_type'], + 'model_has_permissions_permission_model_type_primary'); + } else { + $table->primary([$pivotPermission, $columnNames['model_morph_key'], 'model_type'], + 'model_has_permissions_permission_model_type_primary'); + } + + }); + + Schema::create($tableNames['model_has_roles'], static function (Blueprint $table) use ($tableNames, $columnNames, $pivotRole, $teams) { + $table->unsignedBigInteger($pivotRole); + + $table->string('model_type'); + $table->unsignedBigInteger($columnNames['model_morph_key']); + $table->index([$columnNames['model_morph_key'], 'model_type'], 'model_has_roles_model_id_model_type_index'); + + $table->foreign($pivotRole) + ->references('id') // role id + ->on($tableNames['roles']) + ->onDelete('cascade'); + if ($teams) { + $table->unsignedBigInteger($columnNames['team_foreign_key']); + $table->index($columnNames['team_foreign_key'], 'model_has_roles_team_foreign_key_index'); + + $table->primary([$columnNames['team_foreign_key'], $pivotRole, $columnNames['model_morph_key'], 'model_type'], + 'model_has_roles_role_model_type_primary'); + } else { + $table->primary([$pivotRole, $columnNames['model_morph_key'], 'model_type'], + 'model_has_roles_role_model_type_primary'); + } + }); + + Schema::create($tableNames['role_has_permissions'], static function (Blueprint $table) use ($tableNames, $pivotRole, $pivotPermission) { + $table->unsignedBigInteger($pivotPermission); + $table->unsignedBigInteger($pivotRole); + + $table->foreign($pivotPermission) + ->references('id') // permission id + ->on($tableNames['permissions']) + ->onDelete('cascade'); + + $table->foreign($pivotRole) + ->references('id') // role id + ->on($tableNames['roles']) + ->onDelete('cascade'); + + $table->primary([$pivotPermission, $pivotRole], 'role_has_permissions_permission_id_role_id_primary'); + }); + + app('cache') + ->store(config('permission.cache.store') != 'default' ? config('permission.cache.store') : null) + ->forget(config('permission.cache.key')); + } + + /** + * Reverse the migrations. + */ + public function down(): void + { + $tableNames = config('permission.table_names'); + + throw_if(empty($tableNames), Exception::class, 'Error: config/permission.php not found and defaults could not be merged. Please publish the package configuration before proceeding, or drop the tables manually.'); + + Schema::drop($tableNames['role_has_permissions']); + Schema::drop($tableNames['model_has_roles']); + Schema::drop($tableNames['model_has_permissions']); + Schema::drop($tableNames['roles']); + Schema::drop($tableNames['permissions']); + } +}; diff --git a/phpstan.neon b/phpstan.neon index a71f8a7..88472bf 100644 --- a/phpstan.neon +++ b/phpstan.neon @@ -29,3 +29,18 @@ parameters: noUnnecessaryCollectionCall: true noUnnecessaryCollectionCallOnly: [] noUnnecessaryCollectionCallExcept: [] + + # Ignore errors in vendor migrations + ignoreErrors: + - + message: '#Cannot access offset .* on mixed#' + path: database/migrations/*_create_permission_tables.php + - + message: '#Parameter .* expects .*, mixed given#' + path: database/migrations/*_create_permission_tables.php + - + message: '#Call to an undefined method PHPUnit\\Framework\\TestCase::getJson\(\)#' + path: tests/Feature/* + - + message: '#Cannot call method assert.*\(\) on mixed#' + path: tests/Feature/* diff --git a/tests/Feature/SetTenantMiddlewareTest.php b/tests/Feature/SetTenantMiddlewareTest.php new file mode 100644 index 0000000..4967ce1 --- /dev/null +++ b/tests/Feature/SetTenantMiddlewareTest.php @@ -0,0 +1,148 @@ +group(function (): void { + Route::get('/tenants/{tenant}/test', function (): array { + return ['success' => true, 'tenant_id' => request('tenant_id')]; + })->name('test.tenant'); + + Route::get('/api/test', function (): array { + return ['success' => true, 'tenant_id' => request('tenant_id')]; + })->name('test.header'); + }); +}); + +describe('SetTenant Middleware', function (): void { + it('returns 400 when tenant ID is missing', function (): void { + $response = $this->getJson('/api/test'); + + $response->assertStatus(400) + ->assertJson([ + 'error' => 'Tenant ID is required', + 'message' => 'Please provide tenant ID in path (/tenants/{tenant}) or X-Tenant header', + ]); + }); + + it('returns 404 when tenant does not exist', function (): void { + $response = $this->getJson('/tenants/999/test'); + + $response->assertStatus(404) + ->assertJson([ + 'error' => 'Tenant not found', + 'message' => 'The specified tenant does not exist', + ]); + }); + + it('accepts tenant ID from path parameter', function (): void { + // Create tenant with envelope keys + $keys = TenantKey::generateEnvelopeKeys(); + $tenant = TenantKey::create($keys); + + $response = $this->getJson("/tenants/{$tenant->id}/test"); + + $response->assertStatus(200) + ->assertJson([ + 'success' => true, + 'tenant_id' => $tenant->id, + ]); + }); + + it('accepts tenant ID from X-Tenant header', function (): void { + $keys = TenantKey::generateEnvelopeKeys(); + $tenant = TenantKey::create($keys); + + $response = $this->getJson('/api/test', [ + 'X-Tenant' => (string) $tenant->id, + ]); + + $response->assertStatus(200) + ->assertJson([ + 'success' => true, + 'tenant_id' => $tenant->id, + ]); + }); + + it('prioritizes path parameter over X-Tenant header', function (): void { + $keys1 = TenantKey::generateEnvelopeKeys(); + $tenant1 = TenantKey::create($keys1); + + $keys2 = TenantKey::generateEnvelopeKeys(); + $tenant2 = TenantKey::create($keys2); + + $response = $this->getJson("/tenants/{$tenant1->id}/test", [ + 'X-Tenant' => (string) $tenant2->id, + ]); + + $response->assertStatus(200) + ->assertJson([ + 'success' => true, + 'tenant_id' => $tenant1->id, // Should use path param, not header + ]); + }); + + it('sets Spatie Permission team ID correctly', function (): void { + $keys = TenantKey::generateEnvelopeKeys(); + $tenant = TenantKey::create($keys); + + $this->getJson("/tenants/{$tenant->id}/test"); + + // Verify that PermissionRegistrar has the correct team ID set + $registrar = app(\Spatie\Permission\PermissionRegistrar::class); + expect($registrar->getPermissionsTeamId())->toBe($tenant->id); + }); + + it('prevents cross-tenant access', function (): void { + $keys1 = TenantKey::generateEnvelopeKeys(); + $tenant1 = TenantKey::create($keys1); + + $keys2 = TenantKey::generateEnvelopeKeys(); + $tenant2 = TenantKey::create($keys2); + + // First request sets tenant1 + $this->getJson("/tenants/{$tenant1->id}/test"); + $registrar = app(\Spatie\Permission\PermissionRegistrar::class); + expect($registrar->getPermissionsTeamId())->toBe($tenant1->id); + + // Second request should change to tenant2 + $this->getJson("/tenants/{$tenant2->id}/test"); + expect($registrar->getPermissionsTeamId())->toBe($tenant2->id); + + // Verify it's not still set to tenant1 + expect($registrar->getPermissionsTeamId())->not->toBe($tenant1->id); + }); + + it('handles invalid tenant ID gracefully', function (): void { + $response = $this->getJson('/tenants/invalid/test'); + + // Should treat 'invalid' as 0 and return 404 + $response->assertStatus(404); + }); + + it('stores tenant_id in request for controller access', function (): void { + $keys = TenantKey::generateEnvelopeKeys(); + $tenant = TenantKey::create($keys); + + $response = $this->getJson("/tenants/{$tenant->id}/test"); + + // The test route returns the tenant_id from the request + $response->assertStatus(200) + ->assertJson([ + 'tenant_id' => $tenant->id, + ]); + }); +}); From f67e21d014e088284fddb3e91d687c399f9ada0a Mon Sep 17 00:00:00 2001 From: Holger Schmermbeck Date: Sat, 1 Nov 2025 20:09:43 +0100 Subject: [PATCH 2/3] refactor: make PHPStan ignores more specific for PEST tests - Replace broad Feature/* suppressions with targeted method patterns - Only ignore known Laravel TestCase HTTP methods (getJson, etc.) - Only ignore known Laravel assertion methods (assertStatus, etc.) - Add detailed comment explaining PEST framework PHPStan limitations - Addresses Copilot review feedback for narrower suppressions --- phpstan.neon | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/phpstan.neon b/phpstan.neon index 88472bf..1b31a36 100644 --- a/phpstan.neon +++ b/phpstan.neon @@ -30,7 +30,7 @@ parameters: noUnnecessaryCollectionCallOnly: [] noUnnecessaryCollectionCallExcept: [] - # Ignore errors in vendor migrations + # Ignore errors in vendor migrations (spatie/laravel-permission) ignoreErrors: - message: '#Cannot access offset .* on mixed#' @@ -38,9 +38,12 @@ parameters: - message: '#Parameter .* expects .*, mixed given#' path: database/migrations/*_create_permission_tables.php + + # PEST testing framework limitations: PHPStan cannot infer Laravel TestCase methods + # in PEST's closure-based tests. These are valid Laravel testing methods. - - message: '#Call to an undefined method PHPUnit\\Framework\\TestCase::getJson\(\)#' + message: '#Call to an undefined method PHPUnit\\Framework\\TestCase::(get|post|put|patch|delete|getJson|postJson|putJson|patchJson|deleteJson)\(\)#' path: tests/Feature/* - - message: '#Cannot call method assert.*\(\) on mixed#' + message: '#Cannot call method (assertStatus|assertJson|assertJsonStructure|assertJsonFragment|assertJsonPath|assertExactJson|assertSee|assertDontSee)\(\) on mixed#' path: tests/Feature/* From c5aec28dfa0109f931493b7a25522bce889dfe23 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 1 Nov 2025 19:10:48 +0000 Subject: [PATCH 3/3] Initial plan