From 5afc7e5efcae646dbff974257c743050b77b3f08 Mon Sep 17 00:00:00 2001 From: Holger Schmermbeck Date: Sat, 15 Nov 2025 13:21:15 +0100 Subject: [PATCH 1/2] fix: add idempotency check to role assignment API - Add check in RoleController::store() to prevent duplicate role assignments - Returns 200 OK with existing assignment instead of 500 DB constraint error - Enables idempotency: duplicate requests return same result without side effects - Activate skipped integration test 'role assignment is idempotent' - Add edge case test for temporal parameters on existing assignment Fixes #163 --- app/Http/Controllers/RoleController.php | 26 ++++++++- .../Integration/RbacIntegrationTest.php | 58 +++++++++++++++---- 2 files changed, 72 insertions(+), 12 deletions(-) diff --git a/app/Http/Controllers/RoleController.php b/app/Http/Controllers/RoleController.php index d20fe13..a26cacf 100644 --- a/app/Http/Controllers/RoleController.php +++ b/app/Http/Controllers/RoleController.php @@ -52,7 +52,31 @@ public function store(AssignRoleRequest $request, int $user): JsonResponse /** @var int|null $tenantId */ $tenantId = app(\Spatie\Permission\PermissionRegistrar::class)->getPermissionsTeamId(); - DB::transaction(function () use ($targetUser, $role, $validFrom, $validUntil, $request, $tenantId, $authUser) { + // Idempotency check: Return 200 OK if role already assigned + // This prevents DB unique constraint violations on duplicate assignments + /** @var object{valid_from: ?string, valid_until: ?string, auto_revoke: bool, reason: ?string}|null $existingAssignment */ + $existingAssignment = DB::table('model_has_roles') + ->where('model_type', get_class($targetUser)) + ->where('model_id', $targetUser->id) + ->where('role_id', $role->id) + ->where('tenant_id', $tenantId) + ->first(); + + if ($existingAssignment) { + return response()->json([ + 'message' => 'Role already assigned to user', + 'user_id' => $targetUser->id, + 'role' => $role->name, + 'valid_from' => $existingAssignment->valid_from + ? Carbon::parse($existingAssignment->valid_from)->toIso8601String() + : null, + 'valid_until' => $existingAssignment->valid_until + ? Carbon::parse($existingAssignment->valid_until)->toIso8601String() + : null, + 'auto_revoke' => $existingAssignment->auto_revoke, + 'reason' => $existingAssignment->reason ?? '', + ], Response::HTTP_OK); // 200 OK - Idempotent operation + } DB::transaction(function () use ($targetUser, $role, $validFrom, $validUntil, $request, $tenantId, $authUser) { // Direct database insert to bypass Spatie's relationship methods: // We require additional temporal and audit fields (valid_from, valid_until, auto_revoke, assigned_by, reason) // in the model_has_roles table, which are not supported by Spatie's built-in relationship methods. diff --git a/tests/Feature/Integration/RbacIntegrationTest.php b/tests/Feature/Integration/RbacIntegrationTest.php index faf8d4f..35e1fef 100644 --- a/tests/Feature/Integration/RbacIntegrationTest.php +++ b/tests/Feature/Integration/RbacIntegrationTest.php @@ -270,16 +270,52 @@ ]) ->assertSuccessful(); - // Assign same role again - Phase 3 API does not handle idempotency - // This is a known limitation - duplicate assignment causes DB constraint violation - // Skip this assertion for now - // actingAs($this->admin) - // ->postJson("/api/v1/users/{$user->id}/roles", [ - // 'role' => 'Manager', - // ]) - // ->assertSuccessful(); - - // User should have role + // Assign same role again - should be idempotent (return 200 OK) + actingAs($this->admin) + ->postJson("/api/v1/users/{$user->id}/roles", [ + 'role' => 'Manager', + ]) + ->assertOk() // 200 OK (not 201 Created) + ->assertJson([ + 'message' => 'Role already assigned to user', + 'role' => 'Manager', + ]); + + // User should still have exactly 1 role (not duplicate) expect($user->fresh()->roles)->toHaveCount(1); - })->skip('Phase 3 API does not handle idempotent role assignment'); + }); + + test('idempotency returns existing role with different temporal parameters', function (): void { + $user = User::factory()->create(); + + // Assign permanent role (without temporal constraints) + actingAs($this->admin) + ->postJson("/api/v1/users/{$user->id}/roles", [ + 'role' => 'Manager', + ]) + ->assertCreated(); + + // Try to assign same role again with temporal parameters + // Should return 200 OK with existing assignment unchanged + $response = actingAs($this->admin) + ->postJson("/api/v1/users/{$user->id}/roles", [ + 'role' => 'Manager', + 'valid_from' => now()->toIso8601String(), + 'valid_until' => now()->addDays(7)->toIso8601String(), + ]); + + $response->assertOk() + ->assertJson([ + 'message' => 'Role already assigned to user', + 'role' => 'Manager', + ]); + + // Idempotency: Returns existing assignment, does NOT modify it + // Note: valid_from is set to now() by default even for "permanent" roles + expect($response->json('valid_from'))->not->toBeNull() + ->and($response->json('valid_until'))->toBeNull(); // No expiration + + // User should still have exactly 1 role (not duplicate) + expect($user->fresh()->roles)->toHaveCount(1); + }); }); From f389342552a010848e1fee999e2898b8c592abf2 Mon Sep 17 00:00:00 2001 From: Holger Schmermbeck Date: Sat, 15 Nov 2025 13:44:19 +0100 Subject: [PATCH 2/2] style: fix Pint PSR-12 violations in RoleController --- app/Http/Controllers/RoleController.php | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/app/Http/Controllers/RoleController.php b/app/Http/Controllers/RoleController.php index a26cacf..9ec4399 100644 --- a/app/Http/Controllers/RoleController.php +++ b/app/Http/Controllers/RoleController.php @@ -67,11 +67,11 @@ public function store(AssignRoleRequest $request, int $user): JsonResponse 'message' => 'Role already assigned to user', 'user_id' => $targetUser->id, 'role' => $role->name, - 'valid_from' => $existingAssignment->valid_from - ? Carbon::parse($existingAssignment->valid_from)->toIso8601String() + 'valid_from' => $existingAssignment->valid_from + ? Carbon::parse($existingAssignment->valid_from)->toIso8601String() : null, - 'valid_until' => $existingAssignment->valid_until - ? Carbon::parse($existingAssignment->valid_until)->toIso8601String() + 'valid_until' => $existingAssignment->valid_until + ? Carbon::parse($existingAssignment->valid_until)->toIso8601String() : null, 'auto_revoke' => $existingAssignment->auto_revoke, 'reason' => $existingAssignment->reason ?? '',