diff --git a/app/Http/Controllers/RoleController.php b/app/Http/Controllers/RoleController.php index d20fe13..9ec4399 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); + }); });