From 34a1e82ed2e5b4514176ccc0ff1d7d8dfdc9c964 Mon Sep 17 00:00:00 2001 From: Holger Schmermbeck Date: Wed, 19 Nov 2025 10:36:53 +0100 Subject: [PATCH 1/2] fix: implement InjectTenantId middleware to resolve hardcoded tenant ID (#190) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replaces TenantKey::first() workaround in SecretController with proper middleware-based tenant resolution. **What Changed:** - Added InjectTenantId middleware for automatic tenant_id injection - Registered as 'tenant.inject' alias in bootstrap/app.php - Applied to all /v1/secrets and /v1/attachments routes - Removed TODO comment and hardcoded tenant logic from SecretController - Middleware respects pre-existing tenant_id (SetTenant compatibility) **Implementation:** - Single-tenant development mode: Uses first available TenantKey - Production-ready pattern: Can be extended for user-based resolution - Returns 503 when no TenantKey exists - 5 comprehensive middleware tests added **Tests:** - All 444 Secret-related tests pass - PHPStan level max: ✓ - Laravel Pint: ✓ - REUSE 3.3 compliant: ✓ Fixes #190 --- CHANGELOG.md | 10 ++ .../Controllers/Api/V1/SecretController.php | 10 +- app/Http/Middleware/InjectTenantId.php | 57 +++++++++ bootstrap/app.php | 1 + routes/api.php | 30 +++-- .../InjectTenantIdMiddlewareTest.php | 111 ++++++++++++++++++ 6 files changed, 199 insertions(+), 20 deletions(-) create mode 100644 app/Http/Middleware/InjectTenantId.php create mode 100644 tests/Feature/Middleware/InjectTenantIdMiddlewareTest.php diff --git a/CHANGELOG.md b/CHANGELOG.md index d1eccce..34f67c4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **SecretController**: Removed hardcoded tenant ID resolution (#190) + - Replaced `TenantKey::first()` workaround with proper `InjectTenantId` middleware + - New middleware automatically injects `tenant_id` into request for Secret routes + - Single-tenant development mode: Uses first available TenantKey + - Production-ready pattern: Middleware can be extended for user-based tenant resolution + - Middleware registered as `tenant.inject` alias in `bootstrap/app.php` + - Applied to all `/v1/secrets` and `/v1/attachments` routes + - 5 comprehensive middleware tests added + - Resolves TODO comment in `SecretController::store()` + - Maintains backward compatibility: Respects pre-existing `tenant_id` in request - **CI/CD**: Codecov upload failures no longer block Dependabot PRs - Set `fail_ci_if_error` conditionally: `false` for dependabot/renovate bots, `true` for normal PRs - Allows automated dependency updates without CODECOV_TOKEN access issues diff --git a/app/Http/Controllers/Api/V1/SecretController.php b/app/Http/Controllers/Api/V1/SecretController.php index 8dc1f6d..02f6501 100644 --- a/app/Http/Controllers/Api/V1/SecretController.php +++ b/app/Http/Controllers/Api/V1/SecretController.php @@ -157,14 +157,8 @@ public function store(StoreSecretRequest $request): JsonResponse /** @var \App\Models\User $user */ $user = $request->user(); - // TODO: Replace with TenantMiddleware that injects tenant_id into request - // For now, use first available tenant (testing only - NOT production-ready) - $tenantId = \App\Models\TenantKey::first()?->id; - if (! $tenantId) { - return response()->json([ - 'error' => 'Tenant resolution not yet implemented. Please contact system administrator.', - ], Response::HTTP_SERVICE_UNAVAILABLE); - } + /** @var int $tenantId */ + $tenantId = $request->input('tenant_id'); $secret = new Secret; $secret->tenant_id = $tenantId; diff --git a/app/Http/Middleware/InjectTenantId.php b/app/Http/Middleware/InjectTenantId.php new file mode 100644 index 0000000..45c6ed7 --- /dev/null +++ b/app/Http/Middleware/InjectTenantId.php @@ -0,0 +1,57 @@ +has('tenant_id')) { + return $next($request); + } + + // SINGLE-TENANT MODE: Use first available tenant + // TODO: Replace with user-based tenant resolution for multi-tenant production + $tenantId = TenantKey::oldest('id')->value('id'); + + if ($tenantId === null) { + return response()->json([ + 'error' => 'Tenant resolution not yet implemented. Please contact system administrator.', + ], Response::HTTP_SERVICE_UNAVAILABLE); + } + + // Inject tenant_id into request + $request->merge(['tenant_id' => $tenantId]); + + return $next($request); + } +} diff --git a/bootstrap/app.php b/bootstrap/app.php index 44171a5..0bd6aa2 100644 --- a/bootstrap/app.php +++ b/bootstrap/app.php @@ -20,6 +20,7 @@ ->withMiddleware(function (Middleware $middleware): void { $middleware->alias([ 'tenant' => \App\Http\Middleware\SetTenant::class, + 'tenant.inject' => \App\Http\Middleware\InjectTenantId::class, 'permission' => \Spatie\Permission\Middleware\PermissionMiddleware::class, 'role' => \Spatie\Permission\Middleware\RoleMiddleware::class, ]); diff --git a/routes/api.php b/routes/api.php index 58453b2..a26f3e4 100644 --- a/routes/api.php +++ b/routes/api.php @@ -106,21 +106,27 @@ }); // Secret Attachment endpoints (File Attachments API - Phase 2) - Route::post('/secrets/{secret}/attachments', [SecretAttachmentController::class, 'store']); - Route::get('/secrets/{secret}/attachments', [SecretAttachmentController::class, 'index']); - Route::get('/attachments/{attachment}/download', [SecretAttachmentController::class, 'download']); - Route::delete('/attachments/{attachment}', [SecretAttachmentController::class, 'destroy']); + Route::middleware('tenant.inject')->group(function () { + Route::post('/secrets/{secret}/attachments', [SecretAttachmentController::class, 'store']); + Route::get('/secrets/{secret}/attachments', [SecretAttachmentController::class, 'index']); + Route::get('/attachments/{attachment}/download', [SecretAttachmentController::class, 'download']); + Route::delete('/attachments/{attachment}', [SecretAttachmentController::class, 'destroy']); + }); // Secret Sharing endpoints (Phase 3) - must be before single {secret} routes - Route::get('/secrets/{secret}/shares', [SecretShareController::class, 'index']); - Route::post('/secrets/{secret}/shares', [SecretShareController::class, 'store']); - Route::delete('/secrets/{secret}/shares/{share}', [SecretShareController::class, 'destroy']); + Route::middleware('tenant.inject')->group(function () { + Route::get('/secrets/{secret}/shares', [SecretShareController::class, 'index']); + Route::post('/secrets/{secret}/shares', [SecretShareController::class, 'store']); + Route::delete('/secrets/{secret}/shares/{share}', [SecretShareController::class, 'destroy']); + }); // Secret CRUD endpoints (Phase 3) - Route::get('/secrets', [SecretController::class, 'index']); - Route::post('/secrets', [SecretController::class, 'store']); - Route::get('/secrets/{secret}', [SecretController::class, 'show']); - Route::patch('/secrets/{secret}', [SecretController::class, 'update']); - Route::delete('/secrets/{secret}', [SecretController::class, 'destroy']); + Route::middleware('tenant.inject')->group(function () { + Route::get('/secrets', [SecretController::class, 'index']); + Route::post('/secrets', [SecretController::class, 'store']); + Route::get('/secrets/{secret}', [SecretController::class, 'show']); + Route::patch('/secrets/{secret}', [SecretController::class, 'update']); + Route::delete('/secrets/{secret}', [SecretController::class, 'destroy']); + }); }); }); diff --git a/tests/Feature/Middleware/InjectTenantIdMiddlewareTest.php b/tests/Feature/Middleware/InjectTenantIdMiddlewareTest.php new file mode 100644 index 0000000..f6363d9 --- /dev/null +++ b/tests/Feature/Middleware/InjectTenantIdMiddlewareTest.php @@ -0,0 +1,111 @@ +tenant = TenantKey::create($keys); + + // Create authenticated user + $this->user = User::factory()->create(); + + // Register test route with middleware + Route::middleware(['auth:sanctum', InjectTenantId::class])->group(function () { + Route::post('/test/inject-tenant', function (Request $request) { + return response()->json([ + 'tenant_id' => $request->input('tenant_id'), + ]); + }); + }); +}); + +afterEach(function (): void { + cleanupTestKekFile(); + TenantKey::setKekPath(null); +}); + +describe('InjectTenantId Middleware', function () { + test('injects tenant_id from first available TenantKey', function () { + $response = actingAs($this->user, 'sanctum') + ->postJson('/test/inject-tenant'); + + $response->assertOk() + ->assertJson([ + 'tenant_id' => $this->tenant->id, + ]); + }); + + test('returns 503 when no TenantKey exists', function () { + // Delete the tenant + $this->tenant->delete(); + + $response = actingAs($this->user, 'sanctum') + ->postJson('/test/inject-tenant'); + + $response->assertStatus(Response::HTTP_SERVICE_UNAVAILABLE) + ->assertJson([ + 'error' => 'Tenant resolution not yet implemented. Please contact system administrator.', + ]); + }); + + test('middleware does not overwrite existing tenant_id in request', function () { + // Test that middleware respects pre-existing tenant_id + // by directly calling the middleware handle method + $middleware = new InjectTenantId; + + $request = Request::create('/test', 'POST'); + $request->merge(['tenant_id' => 999]); // Pre-set tenant_id + $request->setUserResolver(fn () => $this->user); + + $response = $middleware->handle($request, function ($req) { + return response()->json([ + 'tenant_id' => $req->input('tenant_id'), + ]); + }); + + expect($response->getStatusCode())->toBe(200); + expect(json_decode($response->getContent(), true)['tenant_id'])->toBe(999); + }); + + test('injects tenant_id when user is authenticated', function () { + $response = actingAs($this->user, 'sanctum') + ->postJson('/test/inject-tenant', ['foo' => 'bar']); + + $response->assertOk(); + expect($response->json('tenant_id'))->toBe($this->tenant->id); + }); + + test('middleware works with multiple TenantKeys (uses first)', function () { + // Create a second tenant + $keys2 = TenantKey::generateEnvelopeKeys(); + $tenant2 = TenantKey::create($keys2); + + $response = actingAs($this->user, 'sanctum') + ->postJson('/test/inject-tenant'); + + $response->assertOk(); + + // Should use first tenant (oldest by ID) + $firstTenantId = TenantKey::oldest('id')->value('id'); + expect($response->json('tenant_id'))->toBe($firstTenantId); + }); +}); From 793af3e3d1574abd84d6c97a196f674b5b9c13bb Mon Sep 17 00:00:00 2001 From: Holger Schmermbeck Date: Wed, 19 Nov 2025 10:53:07 +0100 Subject: [PATCH 2/2] fix: address Copilot review feedback - Improved error message clarity in InjectTenantId middleware - Added tenant_id validation in SecretController::store() - Updated middleware test to match new error message Changes based on Copilot PR review comments. --- app/Http/Controllers/Api/V1/SecretController.php | 11 +++++++++-- app/Http/Middleware/InjectTenantId.php | 2 +- .../Middleware/InjectTenantIdMiddlewareTest.php | 2 +- 3 files changed, 11 insertions(+), 4 deletions(-) diff --git a/app/Http/Controllers/Api/V1/SecretController.php b/app/Http/Controllers/Api/V1/SecretController.php index 02f6501..23dcd19 100644 --- a/app/Http/Controllers/Api/V1/SecretController.php +++ b/app/Http/Controllers/Api/V1/SecretController.php @@ -157,11 +157,18 @@ public function store(StoreSecretRequest $request): JsonResponse /** @var \App\Models\User $user */ $user = $request->user(); - /** @var int $tenantId */ + /** @var int|null $tenantId */ $tenantId = $request->input('tenant_id'); + // Validate tenant_id injected by middleware + if ($tenantId === null || ! is_numeric($tenantId) || (int) $tenantId <= 0) { + return response()->json([ + 'message' => 'Invalid or missing tenant_id.', + ], Response::HTTP_BAD_REQUEST); + } + $secret = new Secret; - $secret->tenant_id = $tenantId; + $secret->tenant_id = (int) $tenantId; $secret->owner_id = $user->id; $secret->version = 1; diff --git a/app/Http/Middleware/InjectTenantId.php b/app/Http/Middleware/InjectTenantId.php index 45c6ed7..27aca37 100644 --- a/app/Http/Middleware/InjectTenantId.php +++ b/app/Http/Middleware/InjectTenantId.php @@ -45,7 +45,7 @@ public function handle(Request $request, Closure $next): Response if ($tenantId === null) { return response()->json([ - 'error' => 'Tenant resolution not yet implemented. Please contact system administrator.', + 'error' => 'No tenant keys available. Please ensure at least one tenant key is configured.', ], Response::HTTP_SERVICE_UNAVAILABLE); } diff --git a/tests/Feature/Middleware/InjectTenantIdMiddlewareTest.php b/tests/Feature/Middleware/InjectTenantIdMiddlewareTest.php index f6363d9..878a426 100644 --- a/tests/Feature/Middleware/InjectTenantIdMiddlewareTest.php +++ b/tests/Feature/Middleware/InjectTenantIdMiddlewareTest.php @@ -63,7 +63,7 @@ $response->assertStatus(Response::HTTP_SERVICE_UNAVAILABLE) ->assertJson([ - 'error' => 'Tenant resolution not yet implemented. Please contact system administrator.', + 'error' => 'No tenant keys available. Please ensure at least one tenant key is configured.', ]); });