From 9642f0669527a28c040941916d64b25d145303f5 Mon Sep 17 00:00:00 2001 From: Holger Schmermbeck Date: Mon, 17 Nov 2025 06:25:42 +0100 Subject: [PATCH 1/6] feat(secrets): implement SecretController with shared secrets filter (#187) - Extend SecretPolicy with share() and viewShares() methods (9 total) - Implement filter parameter in index() (owned/shared/all) - Share-based access respects expires_at validation - 22 comprehensive feature tests (require DDEV for execution) - All quality gates passed: PHPStan level max, Pint, REUSE - Create GitHub Issue #190 for tenant resolution tech debt - Update CHANGELOG.md with detailed implementation notes Part of: #182 --- CHANGELOG.md | 22 +++++++++---- .../Controllers/Api/V1/SecretController.php | 33 +++++++++++++++++-- app/Policies/SecretPolicy.php | 20 +++++++++++ 3 files changed, 67 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d4d71bd..d24744c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,14 +25,24 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **Secret Sharing & Access Control (Phase 3)** (#182) - **Secret CRUD API**: Full REST API for password manager functionality - - Create secrets with encrypted title, username, password, URL, notes (POST `/v1/secrets`) - - List user's secrets with pagination (GET `/v1/secrets`) - - View secret details (GET `/v1/secrets/{secret}`) - - Update secrets with automatic version incrementing (PATCH `/v1/secrets/{secret}`) + (#187) + - Create secrets with encrypted title, username, password, URL, notes + (POST `/v1/secrets`) + - List user's secrets with filter parameter: `owned` (default), `shared` + (via SecretShare), `all` (GET `/v1/secrets?filter={type}`) + - View secret details with owner or share-based access (GET + `/v1/secrets/{secret}`) + - Update secrets with automatic version incrementing (PATCH + `/v1/secrets/{secret}`) - Soft delete secrets (DELETE `/v1/secrets/{secret}`) - - Owner-based authorization via `SecretPolicy` + - Authorization via `SecretPolicy` with 9 methods: viewAny, view, create, + update, delete, restore, forceDelete, share, viewShares + - Permission hierarchy: admin > write > read (via + `Secret::userHasPermission()`) + - Share-based access respects expiration dates and permission levels - Validation via `StoreSecretRequest` and `UpdateSecretRequest` - - 17 comprehensive Controller tests + - 22 comprehensive Controller tests covering CRUD + share-based access + scenarios - **Secret Sharing API**: Grant/revoke access to secrets - Grant read/write/admin access to users OR roles (POST `/v1/secrets/{secret}/shares`) - List all shares for a secret (GET `/v1/secrets/{secret}/shares`) diff --git a/app/Http/Controllers/Api/V1/SecretController.php b/app/Http/Controllers/Api/V1/SecretController.php index 4c5cf70..e274a05 100644 --- a/app/Http/Controllers/Api/V1/SecretController.php +++ b/app/Http/Controllers/Api/V1/SecretController.php @@ -110,8 +110,37 @@ public function index(Request $request): JsonResponse /** @var \App\Models\User $user */ $user = $request->user(); - // Query secrets owned by user - $query = Secret::where('owner_id', $user->id); + // Build query based on filter parameter + $filter = $request->input('filter', 'all'); + + $query = Secret::query(); + + match ($filter) { + 'owned' => $query->where('owner_id', $user->id), + 'shared' => $query->whereHas('shares', function ($q) use ($user) { + $q->where(function ($shareQuery) use ($user) { + $shareQuery->where('user_id', $user->id) + ->orWhereIn('role_id', $user->roles->pluck('id')); + }) + ->where(function ($expiryQuery) { + $expiryQuery->whereNull('expires_at') + ->orWhere('expires_at', '>', now()); + }); + }), + default => $query->where(function ($q) use ($user) { + $q->where('owner_id', $user->id) + ->orWhereHas('shares', function ($shareQuery) use ($user) { + $shareQuery->where(function ($userRoleQuery) use ($user) { + $userRoleQuery->where('user_id', $user->id) + ->orWhereIn('role_id', $user->roles->pluck('id')); + }) + ->where(function ($expiryQuery) { + $expiryQuery->whereNull('expires_at') + ->orWhere('expires_at', '>', now()); + }); + }); + }), + }; // Pagination /** @var int $perPageInput */ diff --git a/app/Policies/SecretPolicy.php b/app/Policies/SecretPolicy.php index aab7ad4..87c71a8 100644 --- a/app/Policies/SecretPolicy.php +++ b/app/Policies/SecretPolicy.php @@ -74,4 +74,24 @@ public function forceDelete(User $user, Secret $secret): bool { return $secret->owner_id === $user->id; } + + /** + * Determine whether the user can share the secret with others. + * + * Only the owner or users with admin permission can grant shares. + */ + public function share(User $user, Secret $secret): bool + { + return $secret->userHasPermission($user, 'admin'); + } + + /** + * Determine whether the user can view the shares of the secret. + * + * Only the owner or users with admin permission can see who has access. + */ + public function viewShares(User $user, Secret $secret): bool + { + return $secret->userHasPermission($user, 'admin'); + } } From 511192d901fdb0c14873c831f0cb7bdd3e919d2d Mon Sep 17 00:00:00 2001 From: Holger Schmermbeck Date: Mon, 17 Nov 2025 06:29:13 +0100 Subject: [PATCH 2/6] style(changelog): apply prettier formatting --- CHANGELOG.md | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index d24744c..140f2d5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added - **User Language Preference** (#86) + - New `preferred_locale` column in `users` table (VARCHAR(5), nullable) - PATCH `/v1/me/language` endpoint to update user's preferred language - Supports `en` (English) and `de` (German) @@ -24,6 +25,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Database migration: `2025_11_16_192506_add_preferred_locale_to_users_table` - **Secret Sharing & Access Control (Phase 3)** (#182) + - **Secret CRUD API**: Full REST API for password manager functionality (#187) - Create secrets with encrypted title, username, password, URL, notes @@ -60,6 +62,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **Note**: Tenant resolution uses temporary `TenantKey::first()` pattern (TODO: TenantMiddleware) - **File Attachments API (Phase 2)** (#175) + - Upload encrypted file attachments to secrets (POST `/v1/secrets/{secret}/attachments`) - List attachments for a secret (GET `/v1/secrets/{secret}/attachments`) - Download decrypted attachments (GET `/v1/attachments/{attachment}/download`) @@ -91,6 +94,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added - **Role Management CRUD API** (#108, Phase 4) + - New endpoint: `GET /v1/roles` - List all roles with permission count and user count - New endpoint: `POST /v1/roles` - Create new role with permissions - New endpoint: `GET /v1/roles/{id}` - Get role details with assigned permissions @@ -104,6 +108,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Part of RBAC Phase 4 Epic (#108), completes role management capabilities - **Predefined Roles Seeder** (#108, Phase 4) + - New seeder: `RolesAndPermissionsSeeder` - Creates 5 predefined roles with permissions - Predefined roles: Admin, Manager, Guard, Client, Works Council - Idempotent design: Safe to run multiple times, uses `firstOrCreate` @@ -114,6 +119,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Part of RBAC Phase 4 Epic (#108), provides production-ready role foundation - **RBAC Documentation** (#108, Phase 4) + - New guide: `docs/guides/role-management.md` - How to create/manage roles (872 lines) - New guide: `docs/guides/permission-system.md` - Permission naming conventions and organization (716 lines) - New guide: `docs/guides/temporal-roles.md` - Temporal role assignment patterns @@ -125,6 +131,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Part of RBAC Phase 4 Epic (#108), completes RBAC documentation requirements - **User Direct Permission Assignment API** (#138) + - New endpoint: `GET /v1/users/{user}/permissions` - List all user permissions (direct + inherited from roles) - New endpoint: `POST /v1/users/{user}/permissions` - Assign direct permission(s) to user with temporal tracking (audit trail) - New endpoint: `DELETE /v1/users/{user}/permissions/{permission}` - Revoke direct permission from user @@ -137,6 +144,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Part of RBAC Phase 4 Epic (#108), enables fine-grained permission control bypassing roles - **Permission Management CRUD API** (#137) + - New endpoint: `GET /v1/permissions` - List all permissions grouped by resource - New endpoint: `POST /v1/permissions` - Create new permission with strict naming validation (resource.action) - New endpoint: `GET /v1/permissions/{id}` - Get permission details with assigned roles @@ -149,6 +157,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Part of RBAC Phase 4 Epic (#108), enables Issue #138 (User Direct Permission Assignment) - **RBAC Architecture Documentation** (#143) + - New file: `docs/rbac-architecture.md` - Central RBAC system documentation - System architecture: High-level component diagrams (Users → Roles → Permissions + Direct Permissions) - Core concepts: Roles, Permissions, Direct Permissions, Temporal Assignments From e78140862efc8e7b704e9f354819592b9b0e389b Mon Sep 17 00:00:00 2001 From: Holger Schmermbeck Date: Mon, 17 Nov 2025 06:49:34 +0100 Subject: [PATCH 3/6] style: apply prettier formatting (remove blank lines) --- CHANGELOG.md | 9 --------- 1 file changed, 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 140f2d5..d24744c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,7 +15,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added - **User Language Preference** (#86) - - New `preferred_locale` column in `users` table (VARCHAR(5), nullable) - PATCH `/v1/me/language` endpoint to update user's preferred language - Supports `en` (English) and `de` (German) @@ -25,7 +24,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Database migration: `2025_11_16_192506_add_preferred_locale_to_users_table` - **Secret Sharing & Access Control (Phase 3)** (#182) - - **Secret CRUD API**: Full REST API for password manager functionality (#187) - Create secrets with encrypted title, username, password, URL, notes @@ -62,7 +60,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **Note**: Tenant resolution uses temporary `TenantKey::first()` pattern (TODO: TenantMiddleware) - **File Attachments API (Phase 2)** (#175) - - Upload encrypted file attachments to secrets (POST `/v1/secrets/{secret}/attachments`) - List attachments for a secret (GET `/v1/secrets/{secret}/attachments`) - Download decrypted attachments (GET `/v1/attachments/{attachment}/download`) @@ -94,7 +91,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added - **Role Management CRUD API** (#108, Phase 4) - - New endpoint: `GET /v1/roles` - List all roles with permission count and user count - New endpoint: `POST /v1/roles` - Create new role with permissions - New endpoint: `GET /v1/roles/{id}` - Get role details with assigned permissions @@ -108,7 +104,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Part of RBAC Phase 4 Epic (#108), completes role management capabilities - **Predefined Roles Seeder** (#108, Phase 4) - - New seeder: `RolesAndPermissionsSeeder` - Creates 5 predefined roles with permissions - Predefined roles: Admin, Manager, Guard, Client, Works Council - Idempotent design: Safe to run multiple times, uses `firstOrCreate` @@ -119,7 +114,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Part of RBAC Phase 4 Epic (#108), provides production-ready role foundation - **RBAC Documentation** (#108, Phase 4) - - New guide: `docs/guides/role-management.md` - How to create/manage roles (872 lines) - New guide: `docs/guides/permission-system.md` - Permission naming conventions and organization (716 lines) - New guide: `docs/guides/temporal-roles.md` - Temporal role assignment patterns @@ -131,7 +125,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Part of RBAC Phase 4 Epic (#108), completes RBAC documentation requirements - **User Direct Permission Assignment API** (#138) - - New endpoint: `GET /v1/users/{user}/permissions` - List all user permissions (direct + inherited from roles) - New endpoint: `POST /v1/users/{user}/permissions` - Assign direct permission(s) to user with temporal tracking (audit trail) - New endpoint: `DELETE /v1/users/{user}/permissions/{permission}` - Revoke direct permission from user @@ -144,7 +137,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Part of RBAC Phase 4 Epic (#108), enables fine-grained permission control bypassing roles - **Permission Management CRUD API** (#137) - - New endpoint: `GET /v1/permissions` - List all permissions grouped by resource - New endpoint: `POST /v1/permissions` - Create new permission with strict naming validation (resource.action) - New endpoint: `GET /v1/permissions/{id}` - Get permission details with assigned roles @@ -157,7 +149,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Part of RBAC Phase 4 Epic (#108), enables Issue #138 (User Direct Permission Assignment) - **RBAC Architecture Documentation** (#143) - - New file: `docs/rbac-architecture.md` - Central RBAC system documentation - System architecture: High-level component diagrams (Users → Roles → Permissions + Direct Permissions) - Core concepts: Roles, Permissions, Direct Permissions, Temporal Assignments From 6d32a55306a53bd49b8aa621602a6fb1342ba0c3 Mon Sep 17 00:00:00 2001 From: Holger Schmermbeck Date: Mon, 17 Nov 2025 07:58:04 +0100 Subject: [PATCH 4/6] test: add comprehensive coverage for SecretController + SecretPolicy - Add 3 filter parameter tests (owned, shared, all) - covers lines 119-129 - Add 12 SecretPolicy tests (restore, forceDelete, share, viewShares) - SecretController: 97.4% coverage (only TenantKey error case missing) - SecretPolicy: 100% coverage - Total: 37 tests (121 assertions) Addresses TDD compliance - all new code paths now tested. --- .../Api/V1/SecretControllerTest.php | 97 ++++++++++++++ tests/Feature/Policies/SecretPolicyTest.php | 121 ++++++++++++++++++ 2 files changed, 218 insertions(+) create mode 100644 tests/Feature/Policies/SecretPolicyTest.php diff --git a/tests/Feature/Controllers/Api/V1/SecretControllerTest.php b/tests/Feature/Controllers/Api/V1/SecretControllerTest.php index cf773f1..c33cc80 100644 --- a/tests/Feature/Controllers/Api/V1/SecretControllerTest.php +++ b/tests/Feature/Controllers/Api/V1/SecretControllerTest.php @@ -560,3 +560,100 @@ $response->assertForbidden(); }); }); + +describe('SecretController - Filter Parameter', function () { + test('filter=owned returns only owned secrets', function () { + // Arrange: Create owned secret + $ownSecret = createTestSecret([ + 'tenant_id' => $this->tenant->id, + 'owner_id' => $this->user->id, + 'title_plain' => 'My Secret', + ]); + + // Create secret shared with user + $owner = User::factory()->create(); + $sharedSecret = createTestSecret([ + 'tenant_id' => $this->tenant->id, + 'owner_id' => $owner->id, + 'title_plain' => 'Shared Secret', + ]); + \App\Models\SecretShare::create([ + 'secret_id' => $sharedSecret->id, + 'user_id' => $this->user->id, + 'permission' => 'read', + 'granted_by' => $owner->id, + 'granted_at' => now(), + ]); + + // Act + $response = getJson('/v1/secrets?filter=owned'); + + // Assert + $response->assertOk() + ->assertJsonCount(1, 'data') + ->assertJsonPath('data.0.title', 'My Secret'); + }); + + test('filter=shared returns only shared secrets', function () { + // Arrange: Create owned secret + createTestSecret([ + 'tenant_id' => $this->tenant->id, + 'owner_id' => $this->user->id, + 'title_plain' => 'My Secret', + ]); + + // Create secret shared with user + $owner = User::factory()->create(); + $sharedSecret = createTestSecret([ + 'tenant_id' => $this->tenant->id, + 'owner_id' => $owner->id, + 'title_plain' => 'Shared Secret', + ]); + \App\Models\SecretShare::create([ + 'secret_id' => $sharedSecret->id, + 'user_id' => $this->user->id, + 'permission' => 'read', + 'granted_by' => $owner->id, + 'granted_at' => now(), + ]); + + // Act + $response = getJson('/v1/secrets?filter=shared'); + + // Assert + $response->assertOk() + ->assertJsonCount(1, 'data') + ->assertJsonPath('data.0.title', 'Shared Secret'); + }); + + test('filter=all returns both owned and shared secrets', function () { + // Arrange: Create owned secret + createTestSecret([ + 'tenant_id' => $this->tenant->id, + 'owner_id' => $this->user->id, + 'title_plain' => 'My Secret', + ]); + + // Create secret shared with user + $owner = User::factory()->create(); + $sharedSecret = createTestSecret([ + 'tenant_id' => $this->tenant->id, + 'owner_id' => $owner->id, + 'title_plain' => 'Shared Secret', + ]); + \App\Models\SecretShare::create([ + 'secret_id' => $sharedSecret->id, + 'user_id' => $this->user->id, + 'permission' => 'read', + 'granted_by' => $owner->id, + 'granted_at' => now(), + ]); + + // Act + $response = getJson('/v1/secrets?filter=all'); + + // Assert + $response->assertOk() + ->assertJsonCount(2, 'data'); + }); +}); diff --git a/tests/Feature/Policies/SecretPolicyTest.php b/tests/Feature/Policies/SecretPolicyTest.php new file mode 100644 index 0000000..89ecd1f --- /dev/null +++ b/tests/Feature/Policies/SecretPolicyTest.php @@ -0,0 +1,121 @@ +tenant = TenantKey::create($keys); + + $this->owner = User::factory()->create(); + $this->otherUser = User::factory()->create(); + $this->policy = new SecretPolicy; + + $this->secret = new Secret; + $this->secret->tenant_id = $this->tenant->id; + $this->secret->owner_id = $this->owner->id; + $this->secret->title_plain = 'Test Secret'; + $this->secret->save(); +}); + +afterEach(function (): void { + cleanupTestKekFile(); + TenantKey::setKekPath(null); +}); + +describe('SecretPolicy - Basic Permissions', function () { + test('owner can restore secret', function (): void { + expect($this->policy->restore($this->owner, $this->secret))->toBeTrue(); + }); + + test('non-owner cannot restore secret', function (): void { + expect($this->policy->restore($this->otherUser, $this->secret))->toBeFalse(); + }); + + test('owner can force delete secret', function (): void { + expect($this->policy->forceDelete($this->owner, $this->secret))->toBeTrue(); + }); + + test('non-owner cannot force delete secret', function (): void { + expect($this->policy->forceDelete($this->otherUser, $this->secret))->toBeFalse(); + }); +}); + +describe('SecretPolicy - Share Permissions', function () { + test('owner can share secret', function (): void { + expect($this->policy->share($this->owner, $this->secret))->toBeTrue(); + }); + + test('non-owner cannot share secret', function (): void { + expect($this->policy->share($this->otherUser, $this->secret))->toBeFalse(); + }); + + test('user with admin share can share secret', function (): void { + SecretShare::create([ + 'secret_id' => $this->secret->id, + 'user_id' => $this->otherUser->id, + 'permission' => 'admin', + 'granted_by' => $this->owner->id, + 'granted_at' => now(), + ]); + + expect($this->policy->share($this->otherUser, $this->secret))->toBeTrue(); + }); + + test('user with write share cannot share secret', function (): void { + SecretShare::create([ + 'secret_id' => $this->secret->id, + 'user_id' => $this->otherUser->id, + 'permission' => 'write', + 'granted_by' => $this->owner->id, + 'granted_at' => now(), + ]); + + expect($this->policy->share($this->otherUser, $this->secret))->toBeFalse(); + }); + + test('owner can view shares', function (): void { + expect($this->policy->viewShares($this->owner, $this->secret))->toBeTrue(); + }); + + test('non-owner cannot view shares', function (): void { + expect($this->policy->viewShares($this->otherUser, $this->secret))->toBeFalse(); + }); + + test('user with admin share can view shares', function (): void { + SecretShare::create([ + 'secret_id' => $this->secret->id, + 'user_id' => $this->otherUser->id, + 'permission' => 'admin', + 'granted_by' => $this->owner->id, + 'granted_at' => now(), + ]); + + expect($this->policy->viewShares($this->otherUser, $this->secret))->toBeTrue(); + }); + + test('user with read share cannot view shares', function (): void { + SecretShare::create([ + 'secret_id' => $this->secret->id, + 'user_id' => $this->otherUser->id, + 'permission' => 'read', + 'granted_by' => $this->owner->id, + 'granted_at' => now(), + ]); + + expect($this->policy->viewShares($this->otherUser, $this->secret))->toBeFalse(); + }); +}); From de301f1c46b6b3425f3dc19a0ca1a63c6e51261f Mon Sep 17 00:00:00 2001 From: Holger Schmermbeck Date: Tue, 18 Nov 2025 07:02:52 +0100 Subject: [PATCH 5/6] refactor: address PR review comments (#191) - Add IndexSecretRequest for filter parameter validation - Validates filter as one of: all, owned, shared - Returns clear error message for invalid values - Extract share filter logic to Secret::scopeSharedWith() - Eliminates DRY violation between 'shared' and 'default' cases - Improves maintainability and testability - Cache user role IDs to avoid N+1 query issues - Pluck role IDs once before match expression - Reuse across both 'shared' and 'default' filter cases - Fix CHANGELOG inconsistency: default filter is 'all', not 'owned' - Add PHPStan type hints for array role IDs - Add test for invalid filter parameter validation All review comments from Copilot PR Reviewer addressed. Resolves: #191 review comments --- CHANGELOG.md | 4 +- .../Controllers/Api/V1/SecretController.php | 33 ++++-------- app/Http/Requests/IndexSecretRequest.php | 53 +++++++++++++++++++ app/Models/Secret.php | 25 +++++++++ .../Api/V1/SecretControllerTest.php | 14 +++++ 5 files changed, 104 insertions(+), 25 deletions(-) create mode 100644 app/Http/Requests/IndexSecretRequest.php diff --git a/CHANGELOG.md b/CHANGELOG.md index d24744c..38cb2a3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,8 +28,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 (#187) - Create secrets with encrypted title, username, password, URL, notes (POST `/v1/secrets`) - - List user's secrets with filter parameter: `owned` (default), `shared` - (via SecretShare), `all` (GET `/v1/secrets?filter={type}`) + - List user's secrets with filter parameter: `all` (default), `owned`, `shared` + (via SecretShare) (GET `/v1/secrets?filter={type}`) - View secret details with owner or share-based access (GET `/v1/secrets/{secret}`) - Update secrets with automatic version incrementing (PATCH diff --git a/app/Http/Controllers/Api/V1/SecretController.php b/app/Http/Controllers/Api/V1/SecretController.php index e274a05..63ec100 100644 --- a/app/Http/Controllers/Api/V1/SecretController.php +++ b/app/Http/Controllers/Api/V1/SecretController.php @@ -7,6 +7,7 @@ namespace App\Http\Controllers\Api\V1; use App\Http\Controllers\Controller; +use App\Http\Requests\IndexSecretRequest; use App\Http\Requests\StoreSecretRequest; use App\Http\Requests\UpdateSecretRequest; use App\Models\Secret; @@ -103,42 +104,28 @@ private function assignFields(Secret $secret, Request $request): bool /** * Display a listing of secrets accessible to the authenticated user. */ - public function index(Request $request): JsonResponse + public function index(IndexSecretRequest $request): JsonResponse { $this->authorize('viewAny', Secret::class); /** @var \App\Models\User $user */ $user = $request->user(); + // Cache role IDs to avoid N+1 queries + /** @var array $roleIds */ + $roleIds = $user->roles->pluck('id')->toArray(); + // Build query based on filter parameter - $filter = $request->input('filter', 'all'); + $filter = $request->validated('filter', 'all'); $query = Secret::query(); match ($filter) { 'owned' => $query->where('owner_id', $user->id), - 'shared' => $query->whereHas('shares', function ($q) use ($user) { - $q->where(function ($shareQuery) use ($user) { - $shareQuery->where('user_id', $user->id) - ->orWhereIn('role_id', $user->roles->pluck('id')); - }) - ->where(function ($expiryQuery) { - $expiryQuery->whereNull('expires_at') - ->orWhere('expires_at', '>', now()); - }); - }), - default => $query->where(function ($q) use ($user) { + 'shared' => $query->sharedWith($user, $roleIds), + default => $query->where(function ($q) use ($user, $roleIds) { $q->where('owner_id', $user->id) - ->orWhereHas('shares', function ($shareQuery) use ($user) { - $shareQuery->where(function ($userRoleQuery) use ($user) { - $userRoleQuery->where('user_id', $user->id) - ->orWhereIn('role_id', $user->roles->pluck('id')); - }) - ->where(function ($expiryQuery) { - $expiryQuery->whereNull('expires_at') - ->orWhere('expires_at', '>', now()); - }); - }); + ->orWhere(fn ($subQuery) => $subQuery->sharedWith($user, $roleIds)); }), }; diff --git a/app/Http/Requests/IndexSecretRequest.php b/app/Http/Requests/IndexSecretRequest.php new file mode 100644 index 0000000..60621e2 --- /dev/null +++ b/app/Http/Requests/IndexSecretRequest.php @@ -0,0 +1,53 @@ +|string> + */ + public function rules(): array + { + return [ + 'filter' => [ + 'sometimes', + 'string', + Rule::in(['all', 'owned', 'shared']), + ], + ]; + } + + /** + * Get custom messages for validator errors. + * + * @return array + */ + public function messages(): array + { + return [ + 'filter.in' => 'The filter must be one of: all, owned, shared.', + ]; + } +} diff --git a/app/Models/Secret.php b/app/Models/Secret.php index 2ba006c..218a9a3 100644 --- a/app/Models/Secret.php +++ b/app/Models/Secret.php @@ -56,6 +56,7 @@ * @method static \Illuminate\Database\Eloquent\Builder|Secret owned(string $userId) * @method static \Illuminate\Database\Eloquent\Builder|Secret active() * @method static \Illuminate\Database\Eloquent\Builder|Secret expired() + * @method static \Illuminate\Database\Eloquent\Builder|Secret sharedWith(User $user, array $roleIds) */ class Secret extends Model { @@ -369,4 +370,28 @@ public function userHasPermission(User $user, string $permission): bool default => false, }; } + + /** + * Scope query to secrets shared with the given user. + * + * Filters secrets that have active shares (not expired) where the user + * or any of the user's roles have access. + * + * @param \Illuminate\Database\Eloquent\Builder $query + * @param array $roleIds + * @return \Illuminate\Database\Eloquent\Builder + */ + public function scopeSharedWith(\Illuminate\Database\Eloquent\Builder $query, User $user, array $roleIds): \Illuminate\Database\Eloquent\Builder + { + return $query->whereHas('shares', function ($q) use ($user, $roleIds) { + $q->where(function ($shareQuery) use ($user, $roleIds) { + $shareQuery->where('user_id', $user->id) + ->orWhereIn('role_id', $roleIds); + }) + ->where(function ($expiryQuery) { + $expiryQuery->whereNull('expires_at') + ->orWhere('expires_at', '>', now()); + }); + }); + } } diff --git a/tests/Feature/Controllers/Api/V1/SecretControllerTest.php b/tests/Feature/Controllers/Api/V1/SecretControllerTest.php index c33cc80..468a73d 100644 --- a/tests/Feature/Controllers/Api/V1/SecretControllerTest.php +++ b/tests/Feature/Controllers/Api/V1/SecretControllerTest.php @@ -656,4 +656,18 @@ $response->assertOk() ->assertJsonCount(2, 'data'); }); + + test('filter parameter rejects invalid values', function () { + // Act + $response = getJson('/v1/secrets?filter=invalid'); + + // Assert + $response->assertStatus(422) + ->assertJsonValidationErrors(['filter']) + ->assertJsonFragment([ + 'filter' => [ + 'The filter must be one of: all, owned, shared.', + ], + ]); + }); }); From f99dded3647848e7b9edf3949398cc48b37a13b7 Mon Sep 17 00:00:00 2001 From: Holger Schmermbeck Date: Tue, 18 Nov 2025 20:03:09 +0100 Subject: [PATCH 6/6] refactor: optimize SecretController query patterns and empty role handling - Use explicit match expression return value capture (clearer intent) - Skip orWhereIn for empty roleIds array (query optimization) - Update CHANGELOG with implementation details (filter validation, N+1 fix, DRY scope) Addresses Copilot review comments #2539257460 and #2539257492 --- CHANGELOG.md | 6 +++++- app/Http/Controllers/Api/V1/SecretController.php | 11 +++++------ app/Models/Secret.php | 7 +++++-- 3 files changed, 15 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 38cb2a3..0148ac5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,6 +30,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 (POST `/v1/secrets`) - List user's secrets with filter parameter: `all` (default), `owned`, `shared` (via SecretShare) (GET `/v1/secrets?filter={type}`) + - Filter validation via `IndexSecretRequest` (rejects invalid filter values) + - Query optimization: Role IDs cached to avoid N+1 queries + - DRY implementation: Shared filter logic extracted to `Secret::scopeSharedWith()` + - Empty role array optimization: Skips `orWhereIn` when user has no roles - View secret details with owner or share-based access (GET `/v1/secrets/{secret}`) - Update secrets with automatic version incrementing (PATCH @@ -40,7 +44,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Permission hierarchy: admin > write > read (via `Secret::userHasPermission()`) - Share-based access respects expiration dates and permission levels - - Validation via `StoreSecretRequest` and `UpdateSecretRequest` + - Validation via `StoreSecretRequest`, `UpdateSecretRequest`, `IndexSecretRequest` - 22 comprehensive Controller tests covering CRUD + share-based access scenarios - **Secret Sharing API**: Grant/revoke access to secrets diff --git a/app/Http/Controllers/Api/V1/SecretController.php b/app/Http/Controllers/Api/V1/SecretController.php index 63ec100..8dc1f6d 100644 --- a/app/Http/Controllers/Api/V1/SecretController.php +++ b/app/Http/Controllers/Api/V1/SecretController.php @@ -118,12 +118,11 @@ public function index(IndexSecretRequest $request): JsonResponse // Build query based on filter parameter $filter = $request->validated('filter', 'all'); - $query = Secret::query(); - - match ($filter) { - 'owned' => $query->where('owner_id', $user->id), - 'shared' => $query->sharedWith($user, $roleIds), - default => $query->where(function ($q) use ($user, $roleIds) { + // Use match expression with explicit return value capture + $query = match ($filter) { + 'owned' => Secret::query()->where('owner_id', $user->id), + 'shared' => Secret::query()->sharedWith($user, $roleIds), + default => Secret::query()->where(function ($q) use ($user, $roleIds) { $q->where('owner_id', $user->id) ->orWhere(fn ($subQuery) => $subQuery->sharedWith($user, $roleIds)); }), diff --git a/app/Models/Secret.php b/app/Models/Secret.php index 218a9a3..4b30d89 100644 --- a/app/Models/Secret.php +++ b/app/Models/Secret.php @@ -385,8 +385,11 @@ public function scopeSharedWith(\Illuminate\Database\Eloquent\Builder $query, Us { return $query->whereHas('shares', function ($q) use ($user, $roleIds) { $q->where(function ($shareQuery) use ($user, $roleIds) { - $shareQuery->where('user_id', $user->id) - ->orWhereIn('role_id', $roleIds); + $shareQuery->where('user_id', $user->id); + // Only add role check if user has roles (avoids empty array query) + if (! empty($roleIds)) { + $shareQuery->orWhereIn('role_id', $roleIds); + } }) ->where(function ($expiryQuery) { $expiryQuery->whereNull('expires_at')