Phase 3 API: Role assignment not idempotent (DB constraint violation)

[kevalyq]

🐛 Problem

Phase 3 role assignment API (POST /api/v1/users/{id}/roles) does not handle idempotent role assignments. Attempting to assign the same role twice to the same user causes a database unique constraint violation.

Error

SQLSTATE[23505]: Unique constraint "model_has_roles_pkey" violated
Key: (tenant_id, role_id, model_id, model_type) already exists

Current Behavior

# First assignment - SUCCESS
POST /api/v1/users/123/roles
{"role": "Manager"}
→ 201 Created

# Second assignment (same role) - FAILS
POST /api/v1/users/123/roles
{"role": "Manager"}
→ 500 Internal Server Error (DB constraint violation)

Expected Behavior

# First assignment
POST /api/v1/users/123/roles
{"role": "Manager"}
→ 201 Created

# Second assignment (idempotent)
POST /api/v1/users/123/roles
{"role": "Manager"}
→ 200 OK (role already assigned, no action needed)
OR
→ 409 Conflict (role already assigned, with clear message)

---

📍 Discovered In

Integration Test: tests/Feature/Integration/RbacIntegrationTest.php

• Test: "role assignment is idempotent"
• Status: SKIPPED (documented Phase 3 limitation)
• PR: fix: resolve permission naming conflict and add RBAC integration tests #162

---

🎯 Root Cause

File: app/Http/Controllers/RoleController.php (Phase 3)

The store() method does not check if the role is already assigned before inserting:

public function store(AssignRoleRequest $request, User $user): JsonResponse
{
    // No idempotency check here!
    $user->assignRole($request->input('role'), [
        'valid_from' => $request->input('valid_from'),
        'valid_until' => $request->input('valid_until'),
        // ...
    ]);
    
    return response()->json(['message' => 'Role assigned'], 201);
}

Database: model_has_roles table has unique constraint:

PRIMARY KEY (tenant_id, role_id, model_id, model_type)

---

✅ Solution

Add idempotency check in RoleController::store():

public function store(AssignRoleRequest $request, User $user): JsonResponse
{
    $roleName = $request->input('role');
    
    // Check if role already assigned
    if ($user->hasRole($roleName)) {
        return response()->json([
            'message' => 'Role already assigned to user',
            'role' => $roleName,
        ], 200); // 200 OK (idempotent)
    }
    
    // Assign role
    $user->assignRole($roleName, [
        'valid_from' => $request->input('valid_from'),
        'valid_until' => $request->input('valid_until'),
        // ...
    ]);
    
    return response()->json(['message' => 'Role assigned'], 201);
}

Alternative: Return 409 Conflict instead of 200 OK if role already assigned (more RESTful).

---

🧪 Acceptance Criteria

• Duplicate role assignment returns 200 OK or 409 Conflict (not 500)
• Integration test "role assignment is idempotent" can be un-skipped
• Existing 277 tests still pass
• API behavior consistent with Phase 4 Permission assignment (also idempotent)
• Documentation updated (if behavior changes from 201 → 200)

---

📊 Impact

Severity: Medium

• Production Risk: Clients may retry failed requests, causing 500 errors
• API Design: Non-idempotent POST is bad practice
• Workaround: Clients must check GET /api/v1/users/{id}/roles before assigning

Users Affected:

• Frontend developers implementing retry logic
• API consumers expecting idempotent behavior
• Integration tests (currently skipped)

---

🔗 Related

Discovered in: PR #162 (Permission Naming Bug Fix)
Test Skipped: tests/Feature/Integration/RbacIntegrationTest.php:268
Phase: Phase 3 (Role Assignment API)
Epic: Issue #5 (RBAC System)

Similar Issues:

• Phase 4 Permission assignment (likely has same issue - needs verification)

---

📝 Implementation Notes

Option 1: Silent Success (200 OK)

• Pros: True idempotency, client doesn't care if role was already assigned
• Cons: Less transparent, harder to debug

Option 2: Explicit Conflict (409 Conflict)

• Pros: Clear feedback, easy to debug
• Cons: Not strictly idempotent (different status codes)
• Message: {"error": "Role 'Manager' is already assigned to this user"}

Recommendation

Option 1 (200 OK) for true idempotency. Most REST APIs treat duplicate assignments as success.

Edge Case: Temporal Roles

If user has permanent Manager role, and client tries to assign temporal Manager role:

• Current: DB constraint violation (same issue)
• Expected: Either reject (409) or update to temporal (PATCH-like behavior)
• Suggested: Return 409 with message: "User already has this role. Use PATCH to modify temporal constraints."

---

Priority: Medium (technical debt, not blocking)
Effort: Small (~30 minutes: add check + test)
Type: Bug / Enhancement

[github-actions]

✅ This issue has been added to the SecPal Feature Roadmap with status: 💡 Ideas

New idea - needs discussion and evaluation

Status Flow:
💡 Ideas → 💬 Discussion → 📥 Backlog → 📋 Planned → 🚧 In Progress → 👀 In Review → ✅ Done

Note: Ideas that won't be implemented should be moved to 🚫 Won't Do with a reason.