From 58c9dd6143223e5fd194ff9bd21b8ab8d8e98b19 Mon Sep 17 00:00:00 2001 From: Holger Schmermbeck Date: Sun, 16 Nov 2025 19:22:05 +0100 Subject: [PATCH 1/3] feat: Secret Sharing & Access Control (Phase 3) - Issue #182 Implements full REST API for password manager Secret CRUD and Sharing functionality. ## Secret CRUD API (5 endpoints) - GET /v1/secrets - List user's secrets with pagination - POST /v1/secrets - Create secret with encrypted fields - GET /v1/secrets/{id} - View secret details - PATCH /v1/secrets/{id} - Update secret with version increment - DELETE /v1/secrets/{id} - Soft delete secret ## Secret Sharing API (3 endpoints) - POST /v1/secrets/{id}/shares - Grant read/write/admin access - GET /v1/secrets/{id}/shares - List all shares - DELETE /v1/secrets/{id}/shares/{shareId} - Revoke access ## Implementation Details - XOR constraint: share with user OR role (not both) - Permission hierarchy: admin > write > read - Optional expiration dates for time-limited access - Owner-based authorization via SecretPolicy and SecretSharePolicy - Validation via StoreSecretRequest, UpdateSecretRequest, GrantShareRequest - Automatic version incrementing on updates ## Test Coverage - 17 SecretController tests (all passing) - 18 SecretShareController tests (all passing) - Total: 390 tests passing (107 Secret-related) ## Known Issues - PHPStan: 14 type warnings in SecretController (tracked in #184) - Tenant resolution uses temporary TenantKey::first() pattern (TODO: TenantMiddleware) ## Breaking Changes None - all new functionality Closes #182 Related: #184 (non-blocking PHPStan warnings) --- CHANGELOG.md | 37 +- .../Controllers/Api/V1/SecretController.php | 209 +++++++++ .../Api/V1/SecretShareController.php | 109 +++++ app/Http/Controllers/Controller.php | 4 +- app/Http/Requests/GrantShareRequest.php | 92 ++++ app/Http/Requests/StoreSecretRequest.php | 57 +++ app/Http/Requests/UpdateSecretRequest.php | 56 +++ app/Policies/SecretPolicy.php | 95 ++++ app/Policies/SecretSharePolicy.php | 56 +++ app/Providers/AppServiceProvider.php | 9 + routes/api.php | 14 + .../Api/V1/SecretControllerTest.php | 424 ++++++++++++++++++ .../Api/V1/SecretShareControllerTest.php | 364 +++++++++++++++ tests/Pest.php | 7 + 14 files changed, 1520 insertions(+), 13 deletions(-) create mode 100644 app/Http/Controllers/Api/V1/SecretController.php create mode 100644 app/Http/Controllers/Api/V1/SecretShareController.php create mode 100644 app/Http/Requests/GrantShareRequest.php create mode 100644 app/Http/Requests/StoreSecretRequest.php create mode 100644 app/Http/Requests/UpdateSecretRequest.php create mode 100644 app/Policies/SecretPolicy.php create mode 100644 app/Policies/SecretSharePolicy.php create mode 100644 tests/Feature/Controllers/Api/V1/SecretControllerTest.php create mode 100644 tests/Feature/Controllers/Api/V1/SecretShareControllerTest.php diff --git a/CHANGELOG.md b/CHANGELOG.md index a64b90b..8c05716 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,18 +14,31 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added -- **Secret Sharing Foundation (Phase 3)** (#182) - - New `secret_shares` table for fine-grained access control - - XOR constraint enforces sharing with either user OR role (not both) - - Permission hierarchy: `admin` > `write` > `read` - - Optional expiration support via `expires_at` timestamp - - `SecretShare` model with UUID primary key, relationships, and scopes - - `Secret.shares()` relationship for access control queries - - `Secret.userHasPermission()` method validates user permissions (owner OR share) - - `active()` scope filters non-expired shares - - Migration tests verify schema integrity (3 tests) - - Model tests cover relationships, scopes, and expiration logic (10 tests) - - Foundation for upcoming Controllers and Policies in separate PR +- **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}`) + - Soft delete secrets (DELETE `/v1/secrets/{secret}`) + - Owner-based authorization via `SecretPolicy` + - Validation via `StoreSecretRequest` and `UpdateSecretRequest` + - 17 comprehensive Controller tests + - **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`) + - Revoke share access (DELETE `/v1/secrets/{secret}/shares/{share}`) + - XOR constraint validation: cannot grant to both user AND role + - Optional expiration dates for time-limited access + - Permission hierarchy: admin > write > read + - Authorization via `SecretSharePolicy` (owner-only for now) + - 18 comprehensive Controller tests covering all scenarios + - **Database Foundation** (already merged): + - `secret_shares` table with XOR constraint + - `SecretShare` model with relationships and scopes + - Migration tests and model tests (13 total) + - **Total Test Coverage**: 35 Controller tests, 13 Model tests, all passing + - **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`) diff --git a/app/Http/Controllers/Api/V1/SecretController.php b/app/Http/Controllers/Api/V1/SecretController.php new file mode 100644 index 0000000..1fe1238 --- /dev/null +++ b/app/Http/Controllers/Api/V1/SecretController.php @@ -0,0 +1,209 @@ + + */ + private function transformSecret(Secret $secret): array + { + return [ + 'id' => $secret->id, + 'title' => $secret->title_plain, + 'username' => $secret->username_plain, + 'password' => $secret->password_plain, + 'url' => $secret->url_plain, + 'notes' => $secret->notes_plain, + 'tags' => $secret->tags, + 'expires_at' => $secret->expires_at?->toIso8601String(), + 'version' => $secret->version, + 'created_at' => $secret->created_at->toIso8601String(), + 'updated_at' => $secret->updated_at->toIso8601String(), + ]; + } + + /** + * Display a listing of secrets accessible to the authenticated user. + */ + 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); + + // Apply filters + if ($request->input('filter') === 'owned') { + // Already filtered by owner_id + } + + // Pagination + /** @var int $perPageInput */ + $perPageInput = $request->input('per_page', 15); + $perPage = min((int) $perPageInput, 100); + $secrets = $query->paginate($perPage); + + // Transform secrets to include plaintext fields + $transformedSecrets = $secrets->getCollection()->map(fn (Secret $secret) => $this->transformSecret($secret)); + + return response()->json([ + 'data' => $transformedSecrets, + 'meta' => [ + 'current_page' => $secrets->currentPage(), + 'per_page' => $secrets->perPage(), + 'total' => $secrets->total(), + ], + ]); + } + + /** + * Store a newly created secret. + */ + 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' => 'No tenant available', + ], Response::HTTP_INTERNAL_SERVER_ERROR); + } + + $secret = new Secret; + $secret->tenant_id = $tenantId; + $secret->owner_id = $user->id; + /** @var string $title */ + $title = $request->input('title'); + $secret->title_plain = $title; + /** @var string|null $username */ + $username = $request->input('username'); + $secret->username_plain = $username; + /** @var string|null $password */ + $password = $request->input('password'); + $secret->password_plain = $password; + /** @var string|null $url */ + $url = $request->input('url'); + $secret->url_plain = $url; + /** @var string|null $notes */ + $notes = $request->input('notes'); + $secret->notes_plain = $notes; + /** @var array|null $tags */ + $tags = $request->input('tags'); + $secret->tags = $tags; + /** @var \Illuminate\Support\Carbon|null $expiresAt */ + $expiresAt = $request->input('expires_at'); + $secret->expires_at = $expiresAt; + $secret->version = 1; + $secret->save(); + + return response()->json([ + 'data' => $this->transformSecret($secret), + ], Response::HTTP_CREATED); + } + + /** + * Display the specified secret. + */ + public function show(Secret $secret): JsonResponse + { + // Authorization handled by SecretPolicy + $this->authorize('view', $secret); + + return response()->json([ + 'data' => $this->transformSecret($secret), + ]); + } + + /** + * Update the specified secret. + */ + public function update(UpdateSecretRequest $request, Secret $secret): JsonResponse + { + // Authorization handled by SecretPolicy + $this->authorize('update', $secret); + + if ($request->has('title')) { + /** @var string $title */ + $title = $request->input('title'); + $secret->title_plain = $title; + } + if ($request->has('username')) { + /** @var string|null $username */ + $username = $request->input('username'); + $secret->username_plain = $username; + } + if ($request->has('password')) { + /** @var string|null $password */ + $password = $request->input('password'); + $secret->password_plain = $password; + } + if ($request->has('url')) { + /** @var string|null $url */ + $url = $request->input('url'); + $secret->url_plain = $url; + } + if ($request->has('notes')) { + /** @var string|null $notes */ + $notes = $request->input('notes'); + $secret->notes_plain = $notes; + } + if ($request->has('tags')) { + /** @var array|null $tags */ + $tags = $request->input('tags'); + $secret->tags = $tags; + } + if ($request->has('expires_at')) { + /** @var \Illuminate\Support\Carbon|null $expiresAt */ + $expiresAt = $request->input('expires_at'); + $secret->expires_at = $expiresAt; + } + + // Increment version on update + $secret->version++; + $secret->save(); + + return response()->json([ + 'data' => $this->transformSecret($secret), + ]); + } + + /** + * Remove the specified secret (soft delete). + */ + public function destroy(Secret $secret): Response + { + // Authorization handled by SecretPolicy + $this->authorize('delete', $secret); + + $secret->delete(); + + return response()->noContent(); + } +} diff --git a/app/Http/Controllers/Api/V1/SecretShareController.php b/app/Http/Controllers/Api/V1/SecretShareController.php new file mode 100644 index 0000000..43da0d0 --- /dev/null +++ b/app/Http/Controllers/Api/V1/SecretShareController.php @@ -0,0 +1,109 @@ +authorize('viewAny', [SecretShare::class, $secret]); + + // Query active (non-expired) shares + $shares = SecretShare::where('secret_id', $secret->id) + ->where(function ($query) { + $query->whereNull('expires_at') + ->orWhere('expires_at', '>', now()); + }) + ->get(); + + // Transform to API response + $data = $shares->map(fn (SecretShare $share) => $this->transformShare($share)); + + return response()->json([ + 'data' => $data, + ]); + } + + /** + * Grant access to a secret. + */ + public function store(GrantShareRequest $request, Secret $secret): JsonResponse + { + // Authorization + $this->authorize('create', [SecretShare::class, $secret]); + + /** @var \App\Models\User $user */ + $user = $request->user(); + + // Create share + $share = SecretShare::create([ + 'secret_id' => $secret->id, + 'user_id' => $request->input('user_id'), + 'role_id' => $request->input('role_id'), + 'permission' => $request->input('permission'), + 'granted_by' => $user->id, + 'granted_at' => now(), + 'expires_at' => $request->input('expires_at'), + ]); + + return response()->json([ + 'data' => $this->transformShare($share), + ], Response::HTTP_CREATED); + } + + /** + * Revoke a share. + */ + public function destroy(Secret $secret, SecretShare $share): Response + { + // Authorization + $this->authorize('delete', [SecretShare::class, $secret, $share]); + + // Delete share + $share->delete(); + + return response()->noContent(); + } + + /** + * Transform SecretShare to API response format. + * + * @return array + */ + private function transformShare(SecretShare $share): array + { + return [ + 'id' => $share->id, + 'secret_id' => $share->secret_id, + 'user_id' => $share->user_id, + 'role_id' => $share->role_id, + 'permission' => $share->permission, + 'granted_by' => $share->granted_by, + 'granted_at' => $share->granted_at->toIso8601String(), + 'expires_at' => $share->expires_at?->toIso8601String(), + ]; + } +} diff --git a/app/Http/Controllers/Controller.php b/app/Http/Controllers/Controller.php index 02fa486..d3c18ed 100644 --- a/app/Http/Controllers/Controller.php +++ b/app/Http/Controllers/Controller.php @@ -5,7 +5,9 @@ namespace App\Http\Controllers; +use Illuminate\Foundation\Auth\Access\AuthorizesRequests; + abstract class Controller { - // + use AuthorizesRequests; } diff --git a/app/Http/Requests/GrantShareRequest.php b/app/Http/Requests/GrantShareRequest.php new file mode 100644 index 0000000..7b3cd4f --- /dev/null +++ b/app/Http/Requests/GrantShareRequest.php @@ -0,0 +1,92 @@ +|string> + */ + public function rules(): array + { + return [ + 'user_id' => [ + 'required_without:role_id', + 'nullable', + 'string', + 'uuid', + 'exists:users,id', + ], + 'role_id' => [ + 'required_without:user_id', + 'nullable', + 'integer', + 'exists:roles,id', + ], + 'permission' => [ + 'required', + 'string', + Rule::in(['read', 'write', 'admin']), + ], + 'expires_at' => [ + 'nullable', + 'date', + 'after:now', + ], + ]; + } + + /** + * Configure the validator instance. + * + * @param \Illuminate\Validation\Validator $validator + */ + public function withValidator($validator): void + { + $validator->after(function (\Illuminate\Validation\Validator $validator): void { + // XOR constraint: user_id and role_id cannot both be present + if ($this->filled('user_id') && $this->filled('role_id')) { + $validator->errors()->add('user_id', 'Cannot grant to both user and role simultaneously (XOR constraint).'); + $validator->errors()->add('role_id', 'Cannot grant to both user and role simultaneously (XOR constraint).'); + } + }); + } + + /** + * Get custom validation messages. + * + * @return array + */ + public function messages(): array + { + return [ + 'user_id.required_without' => 'Either user_id or role_id must be provided.', + 'role_id.required_without' => 'Either user_id or role_id must be provided.', + 'permission.in' => 'Permission must be one of: read, write, admin.', + 'expires_at.after' => 'Expiration date must be in the future.', + ]; + } +} diff --git a/app/Http/Requests/StoreSecretRequest.php b/app/Http/Requests/StoreSecretRequest.php new file mode 100644 index 0000000..f41a20e --- /dev/null +++ b/app/Http/Requests/StoreSecretRequest.php @@ -0,0 +1,57 @@ +> + */ + public function rules(): array + { + return [ + 'title' => ['required', 'string', 'max:255'], + 'username' => ['nullable', 'string', 'max:255'], + 'password' => ['nullable', 'string', 'max:1000'], + 'url' => ['nullable', 'string', 'url', 'max:2048'], + 'notes' => ['nullable', 'string', 'max:10000'], + 'tags' => ['nullable', 'array'], + 'tags.*' => ['string', 'max:50'], + 'expires_at' => ['nullable', 'date', 'after:now'], + ]; + } + + /** + * Get custom error messages for validator errors. + * + * @return array + */ + public function messages(): array + { + return [ + 'title.required' => 'A title is required for the secret.', + 'title.string' => 'The title must be a valid string.', + 'expires_at.after' => 'The expiration date must be in the future.', + 'tags.*.string' => 'Each tag must be a valid string.', + ]; + } +} diff --git a/app/Http/Requests/UpdateSecretRequest.php b/app/Http/Requests/UpdateSecretRequest.php new file mode 100644 index 0000000..5a0898f --- /dev/null +++ b/app/Http/Requests/UpdateSecretRequest.php @@ -0,0 +1,56 @@ +> + */ + public function rules(): array + { + return [ + 'title' => ['sometimes', 'string', 'max:255'], + 'username' => ['nullable', 'string', 'max:255'], + 'password' => ['nullable', 'string', 'max:1000'], + 'url' => ['nullable', 'string', 'url', 'max:2048'], + 'notes' => ['nullable', 'string', 'max:10000'], + 'tags' => ['nullable', 'array'], + 'tags.*' => ['string', 'max:50'], + 'expires_at' => ['nullable', 'date', 'after:now'], + ]; + } + + /** + * Get custom error messages for validator errors. + * + * @return array + */ + public function messages(): array + { + return [ + 'title.string' => 'The title must be a valid string.', + 'expires_at.after' => 'The expiration date must be in the future.', + 'tags.*.string' => 'Each tag must be a valid string.', + ]; + } +} diff --git a/app/Policies/SecretPolicy.php b/app/Policies/SecretPolicy.php new file mode 100644 index 0000000..dede94b --- /dev/null +++ b/app/Policies/SecretPolicy.php @@ -0,0 +1,95 @@ +owner_id === $user->id) { + return true; + } + + // TODO: Check if user has read+ permission via SecretShare + // This will be implemented in Phase 3 (Sharing) + return false; + } + + /** + * Determine whether the user can create secrets. + */ + public function create(User $user): bool + { + return true; // All authenticated users can create secrets + } + + /** + * Determine whether the user can update the secret. + */ + public function update(User $user, Secret $secret): bool + { + // Owner can always update + if ($secret->owner_id === $user->id) { + return true; + } + + // TODO: Check if user has write+ permission via SecretShare + // This will be implemented in Phase 3 (Sharing) + return false; + } + + /** + * Determine whether the user can delete the secret. + */ + public function delete(User $user, Secret $secret): bool + { + // Owner can always delete + if ($secret->owner_id === $user->id) { + return true; + } + + // TODO: Check if user has admin permission via SecretShare + // This will be implemented in Phase 3 (Sharing) + return false; + } + + /** + * Determine whether the user can restore the secret. + */ + public function restore(User $user, Secret $secret): bool + { + return $secret->owner_id === $user->id; + } + + /** + * Determine whether the user can permanently delete the secret. + */ + public function forceDelete(User $user, Secret $secret): bool + { + return $secret->owner_id === $user->id; + } +} diff --git a/app/Policies/SecretSharePolicy.php b/app/Policies/SecretSharePolicy.php new file mode 100644 index 0000000..27dc8b4 --- /dev/null +++ b/app/Policies/SecretSharePolicy.php @@ -0,0 +1,56 @@ +owner_id === $user->id; + } + + /** + * Determine if user can grant access to a secret. + */ + public function create(User $user, Secret $secret): bool + { + // Only owner can grant access + // TODO: Allow users with 'admin' permission on secret + return $secret->owner_id === $user->id; + } + + /** + * Determine if user can revoke a share. + */ + public function delete(User $user, Secret $secret, SecretShare $share): bool + { + // Only owner can revoke shares + // TODO: Allow users with 'admin' permission on secret + // Verify share belongs to this secret + if ($share->secret_id !== $secret->id) { + return false; + } + + return $secret->owner_id === $user->id; + } +} diff --git a/app/Providers/AppServiceProvider.php b/app/Providers/AppServiceProvider.php index c917f78..131c132 100644 --- a/app/Providers/AppServiceProvider.php +++ b/app/Providers/AppServiceProvider.php @@ -9,11 +9,14 @@ use App\Models\Person; use App\Models\Secret; use App\Models\SecretAttachment; +use App\Models\SecretShare; use App\Observers\PersonObserver; use App\Observers\SecretObserver; use App\Policies\PermissionManagementPolicy; use App\Policies\RoleManagementPolicy; use App\Policies\SecretAttachmentPolicy; +use App\Policies\SecretPolicy; +use App\Policies\SecretSharePolicy; use Illuminate\Support\Facades\Gate; use Illuminate\Support\ServiceProvider; use Spatie\Permission\Models\Role; @@ -42,6 +45,12 @@ public function boot(): void // Register policy for Spatie Permission model Gate::policy(Permission::class, PermissionManagementPolicy::class); + // Register policy for Secret model + Gate::policy(Secret::class, SecretPolicy::class); + + // Register policy for SecretShare model + Gate::policy(SecretShare::class, SecretSharePolicy::class); + // Register policy for SecretAttachment model Gate::policy(SecretAttachment::class, SecretAttachmentPolicy::class); diff --git a/routes/api.php b/routes/api.php index a5c1004..bc2da3a 100644 --- a/routes/api.php +++ b/routes/api.php @@ -6,6 +6,8 @@ use App\Http\Controllers\Api\V1\PermissionManagementController; use App\Http\Controllers\Api\V1\RoleManagementController; use App\Http\Controllers\Api\V1\SecretAttachmentController; +use App\Http\Controllers\Api\V1\SecretController; +use App\Http\Controllers\Api\V1\SecretShareController; use App\Http\Controllers\Api\V1\UserPermissionController; use App\Http\Controllers\AuthController; use App\Http\Controllers\PersonController; @@ -107,5 +109,17 @@ Route::get('/secrets/{secret}/attachments', [SecretAttachmentController::class, 'index']); Route::get('/attachments/{attachment}/download', [SecretAttachmentController::class, 'download']); Route::delete('/attachments/{attachment}', [SecretAttachmentController::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']); + + // Secret Sharing endpoints (Phase 3) + 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']); }); }); diff --git a/tests/Feature/Controllers/Api/V1/SecretControllerTest.php b/tests/Feature/Controllers/Api/V1/SecretControllerTest.php new file mode 100644 index 0000000..b22df37 --- /dev/null +++ b/tests/Feature/Controllers/Api/V1/SecretControllerTest.php @@ -0,0 +1,424 @@ +tenant = TenantKey::create($keys); + + // Create authenticated user + $this->user = User::factory()->create(); + actingAs($this->user, 'sanctum'); +}); + +afterEach(function () { + cleanupTestKekFile(); + TenantKey::setKekPath(null); +}); + +describe('SecretController - List Secrets', function () { + test('user can list own secrets', function () { + // Arrange: Create secrets owned by user + $ownSecret1 = createTestSecret([ + 'tenant_id' => $this->tenant->id, + 'owner_id' => $this->user->id, + 'title_plain' => 'My Password', + ]); + $ownSecret2 = createTestSecret([ + 'tenant_id' => $this->tenant->id, + 'owner_id' => $this->user->id, + 'title_plain' => 'API Key', + ]); + + // Create secret by other user (should not appear) + $otherUserKeys = TenantKey::generateEnvelopeKeys(); + $otherTenant = TenantKey::create($otherUserKeys); + $otherUserKeys = TenantKey::generateEnvelopeKeys(); + $otherTenant = TenantKey::create($otherUserKeys); + $otherUser = User::factory()->create(); + createTestSecret([ + 'tenant_id' => $otherTenant->id, + 'owner_id' => $otherUser->id, + 'title_plain' => 'Other Secret', + ]); + + // Act + $response = getJson('/v1/secrets'); + + // Assert + $response->assertOk() + ->assertJsonStructure([ + 'data' => [ + '*' => [ + 'id', + 'title', + 'expires_at', + 'version', + 'created_at', + 'updated_at', + ], + ], + 'meta' => [ + 'current_page', + 'per_page', + 'total', + ], + ]) + ->assertJsonCount(2, 'data') + ->assertJsonPath('data.0.id', $ownSecret1->id) + ->assertJsonPath('data.1.id', $ownSecret2->id); + }); + + test('user can filter secrets by owned status', function () { + // Arrange: Own secret + createTestSecret([ + 'tenant_id' => $this->tenant->id, + 'owner_id' => $this->user->id, + 'title_plain' => 'Owned', + ]); + + // Act + $response = getJson('/v1/secrets?filter=owned'); + + // Assert + $response->assertOk() + ->assertJsonCount(1, 'data'); + }); + + test('list secrets returns paginated results', function () { + // Arrange: Create 15 secrets + foreach (range(1, 15) as $i) { + createTestSecret([ + 'tenant_id' => $this->tenant->id, + 'owner_id' => $this->user->id, + 'title_plain' => "Secret {$i}", + ]); + } + + // Act + $response = getJson('/v1/secrets?per_page=10'); + + // Assert + $response->assertOk() + ->assertJsonCount(10, 'data') + ->assertJsonPath('meta.total', 15) + ->assertJsonPath('meta.per_page', 10); + }); +}); + +describe('SecretController - Create Secret', function () { + test('user can create secret with minimum fields', function () { + // Arrange + $data = [ + 'title' => 'My New Secret', + ]; + + // Act + $response = postJson('/v1/secrets', $data); + + // Assert + $response->assertCreated() + ->assertJsonStructure([ + 'data' => [ + 'id', + 'title', + 'version', + 'created_at', + ], + ]); + + $secretId = $response->json('data.id'); + $secret = Secret::find($secretId); + + expect($secret)->not->toBeNull() + ->and($secret->owner_id)->toBe($this->user->id) + ->and($secret->tenant_id)->toBe($this->tenant->id) + ->and($secret->title_plain)->toBe('My New Secret') + ->and($secret->version)->toBe(1); + }); + + test('user can create secret with all fields', function () { + // Arrange + $data = [ + 'title' => 'Complete Secret', + 'username' => 'john.doe', + 'password' => 'super-secret-123', + 'url' => 'https://secpal.app/login', + 'notes' => 'Important credentials', + 'tags' => ['work', 'production'], + 'expires_at' => '2026-12-31', + ]; + + // Act + $response = postJson('/v1/secrets', $data); + + // Assert + $response->assertCreated(); + + $secret = Secret::find($response->json('data.id')); + expect($secret->title_plain)->toBe('Complete Secret') + ->and($secret->username_plain)->toBe('john.doe') + ->and($secret->password_plain)->toBe('super-secret-123') + ->and($secret->url_plain)->toBe('https://secpal.app/login') + ->and($secret->notes_plain)->toBe('Important credentials') + ->and($secret->tags)->toBe(['work', 'production']) + ->and($secret->expires_at)->toBeInstanceOf(Carbon::class); + }); + + test('title is required when creating secret', function () { + // Arrange + $data = [ + 'username' => 'user', + // title missing + ]; + + // Act + $response = postJson('/v1/secrets', $data); + + // Assert + $response->assertUnprocessable() + ->assertJsonValidationErrors(['title']); + }); + + test('title must be string', function () { + // Arrange + $data = ['title' => 123]; + + // Act + $response = postJson('/v1/secrets', $data); + + // Assert + $response->assertUnprocessable() + ->assertJsonValidationErrors(['title']); + }); + + test('expires_at must be date in future', function () { + // Arrange + $data = [ + 'title' => 'Test', + 'expires_at' => '2020-01-01', // Past date + ]; + + // Act + $response = postJson('/v1/secrets', $data); + + // Assert + $response->assertUnprocessable() + ->assertJsonValidationErrors(['expires_at']); + }); + + test('tags must be array of strings', function () { + // Arrange + $data = [ + 'title' => 'Test', + 'tags' => [123, 456], // Invalid: numbers + ]; + + // Act + $response = postJson('/v1/secrets', $data); + + // Assert + $response->assertUnprocessable() + ->assertJsonValidationErrors(['tags.0', 'tags.1']); + }); +}); + +describe('SecretController - View Secret', function () { + test('user can view own secret', function () { + // Arrange + $secret = createTestSecret([ + 'tenant_id' => $this->tenant->id, + 'owner_id' => $this->user->id, + 'title_plain' => 'My Secret', + 'username_plain' => 'admin', + 'password_plain' => 'pass123', + ]); + + // Act + $response = getJson("/v1/secrets/{$secret->id}"); + + // Assert + $response->assertOk() + ->assertJsonStructure([ + 'data' => [ + 'id', + 'title', + 'username', + 'password', + 'url', + 'notes', + 'tags', + 'expires_at', + 'version', + 'created_at', + 'updated_at', + ], + ]) + ->assertJsonPath('data.id', $secret->id) + ->assertJsonPath('data.title', 'My Secret') + ->assertJsonPath('data.username', 'admin') + ->assertJsonPath('data.password', 'pass123'); + }); + + test('user cannot view others secret', function () { + // Arrange: Secret owned by different user + $otherUserKeys = TenantKey::generateEnvelopeKeys(); + $otherTenant = TenantKey::create($otherUserKeys); + $otherUser = User::factory()->create(); + $secret = createTestSecret([ + 'tenant_id' => $otherTenant->id, + 'owner_id' => $otherUser->id, + 'title_plain' => 'Private Secret', + ]); + + // Act + $response = getJson("/v1/secrets/{$secret->id}"); + + // Assert + $response->assertForbidden(); + }); + + test('viewing nonexistent secret returns 404', function () { + // Arrange: Create a secret to ensure route exists + $secret = createTestSecret([ + 'tenant_id' => $this->tenant->id, + 'owner_id' => $this->user->id, + 'title_plain' => 'Existing', + ]); + + // Act: Try to view non-existent UUID + $response = getJson('/v1/secrets/00000000-0000-0000-0000-000000000000'); + + // Assert + $response->assertNotFound(); + }); +}); + +describe('SecretController - Update Secret', function () { + test('user can update own secret', function () { + // Arrange + $secret = createTestSecret([ + 'tenant_id' => $this->tenant->id, + 'owner_id' => $this->user->id, + 'title_plain' => 'Original Title', + ]); + + $updateData = [ + 'title' => 'Updated Title', + 'username' => 'new-user', + ]; + + // Act + $response = patchJson("/v1/secrets/{$secret->id}", $updateData); + + // Assert + $response->assertOk() + ->assertJsonPath('data.title', 'Updated Title') + ->assertJsonPath('data.username', 'new-user') + ->assertJsonPath('data.version', 2); // Version incremented + }); + + test('updating secret increments version', function () { + // Arrange + $secret = createTestSecret([ + 'tenant_id' => $this->tenant->id, + 'owner_id' => $this->user->id, + 'title_plain' => 'Original', + ]); + + expect($secret->version)->toBe(1); + + // Act + $response = patchJson("/v1/secrets/{$secret->id}", [ + 'title' => 'Updated', + ]); + + // Assert + $response->assertOk(); + $secret->refresh(); + expect($secret->version)->toBe(2); + }); + + test('user cannot update others secret', function () { + // Arrange + $otherUserKeys = TenantKey::generateEnvelopeKeys(); + $otherTenant = TenantKey::create($otherUserKeys); + $otherUser = User::factory()->create(); + $secret = createTestSecret([ + 'tenant_id' => $otherTenant->id, + 'owner_id' => $otherUser->id, + 'title_plain' => 'Private', + ]); + + // Act + $response = patchJson("/v1/secrets/{$secret->id}", [ + 'title' => 'Hacked', + ]); + + // Assert + $response->assertForbidden(); + }); +}); + +describe('SecretController - Delete Secret', function () { + test('user can delete own secret', function () { + // Arrange + $secret = createTestSecret([ + 'tenant_id' => $this->tenant->id, + 'owner_id' => $this->user->id, + 'title_plain' => 'To Delete', + ]); + + // Act + $response = deleteJson("/v1/secrets/{$secret->id}"); + + // Assert + $response->assertNoContent(); + + // Verify soft delete + expect(Secret::find($secret->id))->toBeNull() + ->and(Secret::withTrashed()->find($secret->id))->not->toBeNull(); + }); + + test('user cannot delete others secret', function () { + // Arrange + $otherUserKeys = TenantKey::generateEnvelopeKeys(); + $otherTenant = TenantKey::create($otherUserKeys); + $otherUser = User::factory()->create(); + $secret = createTestSecret([ + 'tenant_id' => $otherTenant->id, + 'owner_id' => $otherUser->id, + 'title_plain' => 'Protected', + ]); + + // Act + $response = deleteJson("/v1/secrets/{$secret->id}"); + + // Assert + $response->assertForbidden(); + + // Verify not deleted + expect(Secret::find($secret->id))->not->toBeNull(); + }); +}); diff --git a/tests/Feature/Controllers/Api/V1/SecretShareControllerTest.php b/tests/Feature/Controllers/Api/V1/SecretShareControllerTest.php new file mode 100644 index 0000000..e22e03c --- /dev/null +++ b/tests/Feature/Controllers/Api/V1/SecretShareControllerTest.php @@ -0,0 +1,364 @@ +tenant = TenantKey::create($keys); + + // Seed roles and permissions + $this->seed(\Database\Seeders\RolesAndPermissionsSeeder::class); + + // Create owner user + $this->owner = User::factory()->create(); + actingAs($this->owner, 'sanctum'); + + // Create a secret owned by owner + $this->secret = createTestSecret([ + 'tenant_id' => $this->tenant->id, + 'owner_id' => $this->owner->id, + 'title_plain' => 'Shared Secret', + ]); + + // Create another user for sharing + $this->otherUser = User::factory()->create(); +}); + +afterEach(function () { + cleanupTestKekFile(); + TenantKey::setKekPath(null); +}); + +describe('SecretShareController - Grant Access', function () { + test('owner can grant read access to user', function () { + // Act + $response = postJson("/v1/secrets/{$this->secret->id}/shares", [ + 'user_id' => $this->otherUser->id, + 'permission' => 'read', + ]); + + // Assert + $response->assertCreated() + ->assertJsonStructure([ + 'data' => [ + 'id', + 'secret_id', + 'user_id', + 'role_id', + 'permission', + 'granted_by', + 'granted_at', + 'expires_at', + ], + ]) + ->assertJsonPath('data.user_id', $this->otherUser->id) + ->assertJsonPath('data.permission', 'read') + ->assertJsonPath('data.granted_by', $this->owner->id); + + // Verify database + expect(SecretShare::count())->toBe(1); + $share = SecretShare::first(); + expect($share->secret_id)->toBe($this->secret->id) + ->and($share->user_id)->toBe($this->otherUser->id) + ->and($share->permission)->toBe('read'); + }); + + test('owner can grant write access to role', function () { + // Arrange + $role = Role::findByName('Manager', 'sanctum'); + + // Act + $response = postJson("/v1/secrets/{$this->secret->id}/shares", [ + 'role_id' => $role->id, + 'permission' => 'write', + ]); + + // Assert + $response->assertCreated() + ->assertJsonPath('data.role_id', $role->id) + ->assertJsonPath('data.permission', 'write') + ->assertJsonPath('data.user_id', null); + + // Verify database + $share = SecretShare::first(); + expect($share->role_id)->toBe($role->id) + ->and($share->user_id)->toBeNull(); + }); + + test('owner can grant admin access', function () { + // Act + $response = postJson("/v1/secrets/{$this->secret->id}/shares", [ + 'user_id' => $this->otherUser->id, + 'permission' => 'admin', + ]); + + // Assert + $response->assertCreated() + ->assertJsonPath('data.permission', 'admin'); + }); + + test('owner can grant with expiration date', function () { + // Arrange + $expiresAt = now()->addDays(7)->toIso8601String(); + + // Act + $response = postJson("/v1/secrets/{$this->secret->id}/shares", [ + 'user_id' => $this->otherUser->id, + 'permission' => 'read', + 'expires_at' => $expiresAt, + ]); + + // Assert + $response->assertCreated() + ->assertJsonPath('data.expires_at', $expiresAt); + }); + + test('cannot grant with both user_id and role_id (XOR constraint)', function () { + // Arrange + $role = Role::findByName('Manager', 'sanctum'); + + // Act + $response = postJson("/v1/secrets/{$this->secret->id}/shares", [ + 'user_id' => $this->otherUser->id, + 'role_id' => $role->id, + 'permission' => 'read', + ]); + + // Assert + $response->assertUnprocessable() + ->assertJsonValidationErrors(['user_id', 'role_id']); + }); + + test('cannot grant without user_id or role_id', function () { + // Act + $response = postJson("/v1/secrets/{$this->secret->id}/shares", [ + 'permission' => 'read', + ]); + + // Assert + $response->assertUnprocessable() + ->assertJsonValidationErrors(['user_id']); + }); + + test('permission is required', function () { + // Act + $response = postJson("/v1/secrets/{$this->secret->id}/shares", [ + 'user_id' => $this->otherUser->id, + ]); + + // Assert + $response->assertUnprocessable() + ->assertJsonValidationErrors(['permission']); + }); + + test('permission must be valid enum (read, write, admin)', function () { + // Act + $response = postJson("/v1/secrets/{$this->secret->id}/shares", [ + 'user_id' => $this->otherUser->id, + 'permission' => 'invalid', + ]); + + // Assert + $response->assertUnprocessable() + ->assertJsonValidationErrors(['permission']); + }); + + test('expires_at must be future date', function () { + // Act + $response = postJson("/v1/secrets/{$this->secret->id}/shares", [ + 'user_id' => $this->otherUser->id, + 'permission' => 'read', + 'expires_at' => now()->subDay()->toIso8601String(), + ]); + + // Assert + $response->assertUnprocessable() + ->assertJsonValidationErrors(['expires_at']); + }); + + test('non-owner cannot grant access', function () { + // Arrange: Act as different user + actingAs($this->otherUser, 'sanctum'); + + // Act + $response = postJson("/v1/secrets/{$this->secret->id}/shares", [ + 'user_id' => User::factory()->create()->id, + 'permission' => 'read', + ]); + + // Assert + $response->assertForbidden(); + }); + + test('cannot grant to non-existent user', function () { + // Act + $response = postJson("/v1/secrets/{$this->secret->id}/shares", [ + 'user_id' => '99999999-9999-9999-9999-999999999999', + 'permission' => 'read', + ]); + + // Assert + $response->assertUnprocessable() + ->assertJsonValidationErrors(['user_id']); + }); + + test('cannot grant to non-existent role', function () { + // Act + $response = postJson("/v1/secrets/{$this->secret->id}/shares", [ + 'role_id' => 99999, + 'permission' => 'read', + ]); + + // Assert + $response->assertUnprocessable() + ->assertJsonValidationErrors(['role_id']); + }); +}); + +describe('SecretShareController - List Shares', function () { + test('owner can list all shares for secret', function () { + // Arrange: Create multiple shares + $role = Role::findByName('Manager', 'sanctum'); + SecretShare::create([ + 'secret_id' => $this->secret->id, + 'user_id' => $this->otherUser->id, + 'permission' => 'read', + 'granted_by' => $this->owner->id, + 'granted_at' => now(), + ]); + SecretShare::create([ + 'secret_id' => $this->secret->id, + 'role_id' => $role->id, + 'permission' => 'write', + 'granted_by' => $this->owner->id, + 'granted_at' => now(), + ]); + + // Act + $response = getJson("/v1/secrets/{$this->secret->id}/shares"); + + // Assert + $response->assertOk() + ->assertJsonCount(2, 'data') + ->assertJsonStructure([ + 'data' => [ + '*' => [ + 'id', + 'secret_id', + 'user_id', + 'role_id', + 'permission', + 'granted_by', + 'granted_at', + 'expires_at', + ], + ], + ]); + }); + + test('non-owner cannot list shares', function () { + // Arrange: Act as different user + actingAs($this->otherUser, 'sanctum'); + + // Act + $response = getJson("/v1/secrets/{$this->secret->id}/shares"); + + // Assert + $response->assertForbidden(); + }); + + test('list shows only non-expired shares', function () { + // Arrange: Create expired and active shares + SecretShare::create([ + 'secret_id' => $this->secret->id, + 'user_id' => $this->otherUser->id, + 'permission' => 'read', + 'granted_by' => $this->owner->id, + 'granted_at' => now(), + 'expires_at' => now()->subDay(), // Expired + ]); + SecretShare::create([ + 'secret_id' => $this->secret->id, + 'user_id' => User::factory()->create()->id, + 'permission' => 'read', + 'granted_by' => $this->owner->id, + 'granted_at' => now(), + 'expires_at' => now()->addDay(), // Active + ]); + + // Act + $response = getJson("/v1/secrets/{$this->secret->id}/shares"); + + // Assert + $response->assertOk() + ->assertJsonCount(1, 'data'); // Only active share + }); +}); + +describe('SecretShareController - Revoke Access', function () { + test('owner can revoke share', function () { + // Arrange: Create share + $share = SecretShare::create([ + 'secret_id' => $this->secret->id, + 'user_id' => $this->otherUser->id, + 'permission' => 'read', + 'granted_by' => $this->owner->id, + 'granted_at' => now(), + ]); + + // Act + $response = deleteJson("/v1/secrets/{$this->secret->id}/shares/{$share->id}"); + + // Assert + $response->assertNoContent(); + expect(SecretShare::count())->toBe(0); + }); + + test('non-owner cannot revoke share', function () { + // Arrange: Create share, then act as different user + $share = SecretShare::create([ + 'secret_id' => $this->secret->id, + 'user_id' => $this->otherUser->id, + 'permission' => 'read', + 'granted_by' => $this->owner->id, + 'granted_at' => now(), + ]); + actingAs($this->otherUser, 'sanctum'); + + // Act + $response = deleteJson("/v1/secrets/{$this->secret->id}/shares/{$share->id}"); + + // Assert + $response->assertForbidden(); + expect(SecretShare::count())->toBe(1); // Share still exists + }); + + test('revoking non-existent share returns 404', function () { + // Act + $response = deleteJson("/v1/secrets/{$this->secret->id}/shares/99999999-9999-9999-9999-999999999999"); + + // Assert + $response->assertNotFound(); + }); +}); diff --git a/tests/Pest.php b/tests/Pest.php index 72ffe19..24e73a8 100644 --- a/tests/Pest.php +++ b/tests/Pest.php @@ -108,6 +108,13 @@ function createTestSecret(array $attributes): \App\Models\Secret $secret->tenant_id = $attributes['tenant_id']; $secret->owner_id = $attributes['owner_id']; $secret->title_plain = $attributes['title_plain'] ?? 'Test Secret'; + $secret->username_plain = $attributes['username_plain'] ?? null; + $secret->password_plain = $attributes['password_plain'] ?? null; + $secret->url_plain = $attributes['url_plain'] ?? null; + $secret->notes_plain = $attributes['notes_plain'] ?? null; + $secret->tags = $attributes['tags'] ?? null; + $secret->expires_at = $attributes['expires_at'] ?? null; + $secret->version = $attributes['version'] ?? 1; $secret->save(); return $secret; From cdb936019b53c8e12e458efb2846afbeaf1abc53 Mon Sep 17 00:00:00 2001 From: Holger Schmermbeck Date: Sun, 16 Nov 2025 19:47:27 +0100 Subject: [PATCH 2/3] fix: Resolve all Copilot review feedback - SecretPolicy: Implement userHasPermission() calls (CRITICAL) - SecretController: Add viewAny authorization, fix error handling - Tests: Add 6 share-based access tests (read/write/admin permissions) - Tests: Remove duplicate code - SecretController: Fix DocBlock accuracy, improve error messages Fixes all 11 Copilot review comments (8 functional, 3 nitpicks addressed). All 23 SecretController tests + 18 SecretShare tests passing. --- .../Controllers/Api/V1/SecretController.php | 14 +- app/Policies/SecretPolicy.php | 30 +--- .../Api/V1/SecretControllerTest.php | 158 +++++++++++++++++- 3 files changed, 168 insertions(+), 34 deletions(-) diff --git a/app/Http/Controllers/Api/V1/SecretController.php b/app/Http/Controllers/Api/V1/SecretController.php index 1fe1238..f558d6f 100644 --- a/app/Http/Controllers/Api/V1/SecretController.php +++ b/app/Http/Controllers/Api/V1/SecretController.php @@ -18,7 +18,8 @@ * SecretController handles CRUD operations for Secrets. * * All secrets are scoped to the authenticated user's tenant. - * Users can only access secrets they own or have explicit share access to. + * Users can access secrets they own (owner-based) only. + * Shared secret access via SecretShare is checked at Policy level. */ class SecretController extends Controller { @@ -49,17 +50,14 @@ private function transformSecret(Secret $secret): array */ public function index(Request $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); - // Apply filters - if ($request->input('filter') === 'owned') { - // Already filtered by owner_id - } - // Pagination /** @var int $perPageInput */ $perPageInput = $request->input('per_page', 15); @@ -92,8 +90,8 @@ public function store(StoreSecretRequest $request): JsonResponse $tenantId = \App\Models\TenantKey::first()?->id; if (! $tenantId) { return response()->json([ - 'error' => 'No tenant available', - ], Response::HTTP_INTERNAL_SERVER_ERROR); + 'error' => 'Tenant resolution not yet implemented. Please contact system administrator.', + ], Response::HTTP_SERVICE_UNAVAILABLE); } $secret = new Secret; diff --git a/app/Policies/SecretPolicy.php b/app/Policies/SecretPolicy.php index dede94b..aab7ad4 100644 --- a/app/Policies/SecretPolicy.php +++ b/app/Policies/SecretPolicy.php @@ -29,14 +29,8 @@ public function viewAny(User $user): bool */ public function view(User $user, Secret $secret): bool { - // Owner can always view - if ($secret->owner_id === $user->id) { - return true; - } - - // TODO: Check if user has read+ permission via SecretShare - // This will be implemented in Phase 3 (Sharing) - return false; + // Check via userHasPermission (handles owner + share access) + return $secret->userHasPermission($user, 'read'); } /** @@ -52,14 +46,8 @@ public function create(User $user): bool */ public function update(User $user, Secret $secret): bool { - // Owner can always update - if ($secret->owner_id === $user->id) { - return true; - } - - // TODO: Check if user has write+ permission via SecretShare - // This will be implemented in Phase 3 (Sharing) - return false; + // Check via userHasPermission (handles owner + share access) + return $secret->userHasPermission($user, 'write'); } /** @@ -67,14 +55,8 @@ public function update(User $user, Secret $secret): bool */ public function delete(User $user, Secret $secret): bool { - // Owner can always delete - if ($secret->owner_id === $user->id) { - return true; - } - - // TODO: Check if user has admin permission via SecretShare - // This will be implemented in Phase 3 (Sharing) - return false; + // Check via userHasPermission (handles owner + share access) + return $secret->userHasPermission($user, 'admin'); } /** diff --git a/tests/Feature/Controllers/Api/V1/SecretControllerTest.php b/tests/Feature/Controllers/Api/V1/SecretControllerTest.php index b22df37..4273f37 100644 --- a/tests/Feature/Controllers/Api/V1/SecretControllerTest.php +++ b/tests/Feature/Controllers/Api/V1/SecretControllerTest.php @@ -54,8 +54,6 @@ // Create secret by other user (should not appear) $otherUserKeys = TenantKey::generateEnvelopeKeys(); $otherTenant = TenantKey::create($otherUserKeys); - $otherUserKeys = TenantKey::generateEnvelopeKeys(); - $otherTenant = TenantKey::create($otherUserKeys); $otherUser = User::factory()->create(); createTestSecret([ 'tenant_id' => $otherTenant->id, @@ -422,3 +420,159 @@ expect(Secret::find($secret->id))->not->toBeNull(); }); }); + +describe('SecretController - Share-Based Access', function () { + test('user with read share can view secret', function () { + // Arrange: Create secret owned by different user + $owner = User::factory()->create(); + $secret = createTestSecret([ + 'tenant_id' => $this->tenant->id, + 'owner_id' => $owner->id, + 'title_plain' => 'Shared Secret', + 'password_plain' => 'shared-pass', + ]); + + // Grant read access to current user + \App\Models\SecretShare::create([ + 'secret_id' => $secret->id, + 'user_id' => $this->user->id, + 'permission' => 'read', + 'granted_by' => $owner->id, + 'granted_at' => now(), + ]); + + // Act + $response = getJson("/v1/secrets/{$secret->id}"); + + // Assert + $response->assertOk() + ->assertJsonPath('data.title', 'Shared Secret') + ->assertJsonPath('data.password', 'shared-pass'); + }); + + test('user with read share cannot update secret', function () { + // Arrange + $owner = User::factory()->create(); + $secret = createTestSecret([ + 'tenant_id' => $this->tenant->id, + 'owner_id' => $owner->id, + 'title_plain' => 'Read Only', + ]); + + \App\Models\SecretShare::create([ + 'secret_id' => $secret->id, + 'user_id' => $this->user->id, + 'permission' => 'read', + 'granted_by' => $owner->id, + 'granted_at' => now(), + ]); + + // Act + $response = patchJson("/v1/secrets/{$secret->id}", [ + 'title' => 'Hacked', + ]); + + // Assert + $response->assertForbidden(); + }); + + test('user with write share can update secret', function () { + // Arrange + $owner = User::factory()->create(); + $secret = createTestSecret([ + 'tenant_id' => $this->tenant->id, + 'owner_id' => $owner->id, + 'title_plain' => 'Editable', + ]); + + \App\Models\SecretShare::create([ + 'secret_id' => $secret->id, + 'user_id' => $this->user->id, + 'permission' => 'write', + 'granted_by' => $owner->id, + 'granted_at' => now(), + ]); + + // Act + $response = patchJson("/v1/secrets/{$secret->id}", [ + 'title' => 'Updated by Shared User', + ]); + + // Assert + $response->assertOk() + ->assertJsonPath('data.title', 'Updated by Shared User'); + }); + + test('user with write share cannot delete secret', function () { + // Arrange + $owner = User::factory()->create(); + $secret = createTestSecret([ + 'tenant_id' => $this->tenant->id, + 'owner_id' => $owner->id, + 'title_plain' => 'Protected', + ]); + + \App\Models\SecretShare::create([ + 'secret_id' => $secret->id, + 'user_id' => $this->user->id, + 'permission' => 'write', + 'granted_by' => $owner->id, + 'granted_at' => now(), + ]); + + // Act + $response = deleteJson("/v1/secrets/{$secret->id}"); + + // Assert + $response->assertForbidden(); + }); + + test('user with admin share can delete secret', function () { + // Arrange + $owner = User::factory()->create(); + $secret = createTestSecret([ + 'tenant_id' => $this->tenant->id, + 'owner_id' => $owner->id, + 'title_plain' => 'Deletable', + ]); + + \App\Models\SecretShare::create([ + 'secret_id' => $secret->id, + 'user_id' => $this->user->id, + 'permission' => 'admin', + 'granted_by' => $owner->id, + 'granted_at' => now(), + ]); + + // Act + $response = deleteJson("/v1/secrets/{$secret->id}"); + + // Assert + $response->assertNoContent(); + }); + + test('expired share does not grant access', function () { + // Arrange + $owner = User::factory()->create(); + $secret = createTestSecret([ + 'tenant_id' => $this->tenant->id, + 'owner_id' => $owner->id, + 'title_plain' => 'Expired Access', + ]); + + \App\Models\SecretShare::create([ + 'secret_id' => $secret->id, + 'user_id' => $this->user->id, + 'permission' => 'read', + 'granted_by' => $owner->id, + 'granted_at' => now()->subDays(10), + 'expires_at' => now()->subDay(), // Expired yesterday + ]); + + // Act + $response = getJson("/v1/secrets/{$secret->id}"); + + // Assert + $response->assertForbidden(); + }); +}); From 4667552916b9e7377eeb261198e228ba5c7582c6 Mon Sep 17 00:00:00 2001 From: Holger Schmermbeck Date: Sun, 16 Nov 2025 20:03:00 +0100 Subject: [PATCH 3/3] refactor: Address all Copilot review nitpicks - Add missing create authorization in store() - Use active() scope in SecretShareController - Remove useless filter test (filter=owned not implemented) - Fix route ordering (nested routes before parameterized) - Fix version inflation (only increment if modified) - Refactor field assignment to private helper method - Reduce code duplication in store() and update() All 53 Secret/SecretShare tests passing (211 assertions). --- .../Controllers/Api/V1/SecretController.php | 124 +++++++++--------- .../Api/V1/SecretShareController.php | 5 +- routes/api.php | 10 +- .../Api/V1/SecretControllerTest.php | 16 --- 4 files changed, 71 insertions(+), 84 deletions(-) diff --git a/app/Http/Controllers/Api/V1/SecretController.php b/app/Http/Controllers/Api/V1/SecretController.php index f558d6f..4c5cf70 100644 --- a/app/Http/Controllers/Api/V1/SecretController.php +++ b/app/Http/Controllers/Api/V1/SecretController.php @@ -45,6 +45,61 @@ private function transformSecret(Secret $secret): array ]; } + /** + * Assign request fields to secret model. + * + * @return bool Whether any field was modified + */ + private function assignFields(Secret $secret, Request $request): bool + { + $modified = false; + + if ($request->has('title')) { + /** @var string $value */ + $value = $request->input('title'); + $secret->title_plain = $value; + $modified = true; + } + if ($request->has('username')) { + /** @var string|null $value */ + $value = $request->input('username'); + $secret->username_plain = $value; + $modified = true; + } + if ($request->has('password')) { + /** @var string|null $value */ + $value = $request->input('password'); + $secret->password_plain = $value; + $modified = true; + } + if ($request->has('url')) { + /** @var string|null $value */ + $value = $request->input('url'); + $secret->url_plain = $value; + $modified = true; + } + if ($request->has('notes')) { + /** @var string|null $value */ + $value = $request->input('notes'); + $secret->notes_plain = $value; + $modified = true; + } + if ($request->has('tags')) { + /** @var array|null $value */ + $value = $request->input('tags'); + $secret->tags = $value; + $modified = true; + } + if ($request->has('expires_at')) { + /** @var \Illuminate\Support\Carbon|null $value */ + $value = $request->input('expires_at'); + $secret->expires_at = $value; + $modified = true; + } + + return $modified; + } + /** * Display a listing of secrets accessible to the authenticated user. */ @@ -82,6 +137,8 @@ public function index(Request $request): JsonResponse */ public function store(StoreSecretRequest $request): JsonResponse { + $this->authorize('create', Secret::class); + /** @var \App\Models\User $user */ $user = $request->user(); @@ -97,28 +154,9 @@ public function store(StoreSecretRequest $request): JsonResponse $secret = new Secret; $secret->tenant_id = $tenantId; $secret->owner_id = $user->id; - /** @var string $title */ - $title = $request->input('title'); - $secret->title_plain = $title; - /** @var string|null $username */ - $username = $request->input('username'); - $secret->username_plain = $username; - /** @var string|null $password */ - $password = $request->input('password'); - $secret->password_plain = $password; - /** @var string|null $url */ - $url = $request->input('url'); - $secret->url_plain = $url; - /** @var string|null $notes */ - $notes = $request->input('notes'); - $secret->notes_plain = $notes; - /** @var array|null $tags */ - $tags = $request->input('tags'); - $secret->tags = $tags; - /** @var \Illuminate\Support\Carbon|null $expiresAt */ - $expiresAt = $request->input('expires_at'); - $secret->expires_at = $expiresAt; $secret->version = 1; + + $this->assignFields($secret, $request); $secret->save(); return response()->json([ @@ -147,45 +185,13 @@ public function update(UpdateSecretRequest $request, Secret $secret): JsonRespon // Authorization handled by SecretPolicy $this->authorize('update', $secret); - if ($request->has('title')) { - /** @var string $title */ - $title = $request->input('title'); - $secret->title_plain = $title; - } - if ($request->has('username')) { - /** @var string|null $username */ - $username = $request->input('username'); - $secret->username_plain = $username; - } - if ($request->has('password')) { - /** @var string|null $password */ - $password = $request->input('password'); - $secret->password_plain = $password; - } - if ($request->has('url')) { - /** @var string|null $url */ - $url = $request->input('url'); - $secret->url_plain = $url; - } - if ($request->has('notes')) { - /** @var string|null $notes */ - $notes = $request->input('notes'); - $secret->notes_plain = $notes; - } - if ($request->has('tags')) { - /** @var array|null $tags */ - $tags = $request->input('tags'); - $secret->tags = $tags; - } - if ($request->has('expires_at')) { - /** @var \Illuminate\Support\Carbon|null $expiresAt */ - $expiresAt = $request->input('expires_at'); - $secret->expires_at = $expiresAt; - } + $modified = $this->assignFields($secret, $request); - // Increment version on update - $secret->version++; - $secret->save(); + if ($modified) { + // Increment version on update + $secret->version++; + $secret->save(); + } return response()->json([ 'data' => $this->transformSecret($secret), diff --git a/app/Http/Controllers/Api/V1/SecretShareController.php b/app/Http/Controllers/Api/V1/SecretShareController.php index 43da0d0..4fb5438 100644 --- a/app/Http/Controllers/Api/V1/SecretShareController.php +++ b/app/Http/Controllers/Api/V1/SecretShareController.php @@ -33,10 +33,7 @@ public function index(Secret $secret): JsonResponse // Query active (non-expired) shares $shares = SecretShare::where('secret_id', $secret->id) - ->where(function ($query) { - $query->whereNull('expires_at') - ->orWhere('expires_at', '>', now()); - }) + ->active() ->get(); // Transform to API response diff --git a/routes/api.php b/routes/api.php index bc2da3a..f76c4ca 100644 --- a/routes/api.php +++ b/routes/api.php @@ -110,16 +110,16 @@ 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']); + // 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']); - - // Secret Sharing endpoints (Phase 3) - 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']); }); }); diff --git a/tests/Feature/Controllers/Api/V1/SecretControllerTest.php b/tests/Feature/Controllers/Api/V1/SecretControllerTest.php index 4273f37..cf773f1 100644 --- a/tests/Feature/Controllers/Api/V1/SecretControllerTest.php +++ b/tests/Feature/Controllers/Api/V1/SecretControllerTest.php @@ -88,22 +88,6 @@ ->assertJsonPath('data.1.id', $ownSecret2->id); }); - test('user can filter secrets by owned status', function () { - // Arrange: Own secret - createTestSecret([ - 'tenant_id' => $this->tenant->id, - 'owner_id' => $this->user->id, - 'title_plain' => 'Owned', - ]); - - // Act - $response = getJson('/v1/secrets?filter=owned'); - - // Assert - $response->assertOk() - ->assertJsonCount(1, 'data'); - }); - test('list secrets returns paginated results', function () { // Arrange: Create 15 secrets foreach (range(1, 15) as $i) {