diff --git a/CHANGELOG.md b/CHANGELOG.md index d4d71bd..0148ac5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,14 +25,28 @@ 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: `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 + `/v1/secrets/{secret}`) - Soft delete secrets (DELETE `/v1/secrets/{secret}`) - - Owner-based authorization via `SecretPolicy` - - Validation via `StoreSecretRequest` and `UpdateSecretRequest` - - 17 comprehensive Controller tests + - 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`, `UpdateSecretRequest`, `IndexSecretRequest` + - 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..8dc1f6d 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,15 +104,29 @@ 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(); - // Query secrets owned by user - $query = Secret::where('owner_id', $user->id); + // 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->validated('filter', 'all'); + + // 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)); + }), + }; // Pagination /** @var int $perPageInput */ 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..4b30d89 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,31 @@ 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); + // 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') + ->orWhere('expires_at', '>', now()); + }); + }); + } } 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'); + } } diff --git a/tests/Feature/Controllers/Api/V1/SecretControllerTest.php b/tests/Feature/Controllers/Api/V1/SecretControllerTest.php index cf773f1..468a73d 100644 --- a/tests/Feature/Controllers/Api/V1/SecretControllerTest.php +++ b/tests/Feature/Controllers/Api/V1/SecretControllerTest.php @@ -560,3 +560,114 @@ $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'); + }); + + 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.', + ], + ]); + }); +}); 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(); + }); +});