[DRAFT] feat: RBAC Phase 4 - Role Management CRUD API

[kevalyq]

🎯 Objective

Implement RESTful Role Management CRUD API as part of RBAC Phase 4 (#108).

📊 Status

⚠️ DRAFT - Blocked by #125

This PR is in DRAFT status because tests are currently failing due to Laravel Guard architecture mismatch. See blocking issue #125 for full technical context.

🚧 Blocking Issue

#125: [EPIC] Migrate Permission System from 'web' to 'sanctum' Guard

Problem: Permissions created with default guard_name='web', but User authenticated via auth:sanctum middleware. Spatie Laravel-Permission checks fail due to guard mismatch.

Impact: 36 tests failing (3 in this PR's test file, 33 in existing tests)

Solution: Systematic migration to explicit guard_name='sanctum' across all test files and User model.

✅ What's Implemented

1. Controller (app/Http/Controllers/Api/V1/RoleManagementController.php)

Full RESTful CRUD implementation:

• index() - List all roles with permission counts
• store(CreateRoleRequest) - Create role with permissions
• show($id) - Get role detail with permissions
• update(UpdateRoleRequest, $id) - Update role name/permissions
• destroy($id) - Delete role (with safety check for assigned users)

Quality Gates:

• ✅ PHPStan Level Max: 0 errors
• ✅ Laravel Pint: Clean
• ✅ Policy-based authorization on all methods

2. Form Requests

CreateRoleRequest.php (66 lines):

• Validates role name (unique, max 255)
• Validates description (nullable, max 1000)
• Custom permission existence validation via closure
• Authorization via RoleManagementPolicy::create()

UpdateRoleRequest.php (71 lines):

• Same validation as CreateRoleRequest
• Unique rule excludes current role ID
• Authorization via RoleManagementPolicy::update()

3. Policy (app/Policies/RoleManagementPolicy.php)

5 authorization methods:

• viewAny() → checks roles.read
• view() → checks roles.read
• create() → checks roles.create
• update() → checks roles.update
• delete() → checks roles.delete

Registered in AppServiceProvider via Gate::policy().

4. Routes (routes/api.php)

5 RESTful endpoints under /v1/roles:

GET    /api/v1/roles           # List roles
POST   /api/v1/roles           # Create role
GET    /api/v1/roles/{id}      # Get role detail
PATCH  /api/v1/roles/{id}      # Update role
DELETE /api/v1/roles/{id}      # Delete role

All protected with auth:sanctum middleware.

5. Auth Config (config/auth.php)

Added sanctum guard configuration:

'sanctum' => [
    'driver' => 'sanctum',
    'provider' => 'users',
],

Purpose: Enables $role->users()->count() to work correctly in controller.

6. Tests (tests/Feature/RoleManagementApiTest.php)

26 comprehensive tests covering:

• ✅ 401 Unauthorized (no token)
• ✅ 403 Forbidden (missing permissions)
• ✅ 422 Validation errors (all edge cases)
• ✅ 404 Not Found (invalid role ID)
• ⚠️ 200/201/204 Success responses (3 FAILING - blocked by [EPIC] Migrate Permission System from 'web' to 'sanctum' Guard #125)

Test Status:

• 23 passing (authorization, validation, error handling)
• 3 failing (permission checks fail due to guard mismatch)

Failing Tests:

1. GET /v1/roles - List Roles → returns list of roles with permissions
2. GET /v1/roles/{id} - Get Role Details → returns role with permissions
3. PATCH /v1/roles/{id} - Update Role → updates role successfully

Error Message:

PermissionDoesNotExist: There is no permission named `employees.read` for guard `sanctum`.

🔧 Technical Implementation Details

Safety Features

• Delete Protection: Cannot delete role if assigned to users (returns 422)
• Policy Authorization: All methods check permissions before execution
• Type Safety: Null-safe operators, proper PHPDoc annotations
• Validation: Comprehensive input validation in Form Requests

Database Interactions

• Uses Spatie Role::withCount('permissions') for efficient loading
• Checks $role->users()->count() before deletion
• Syncs permissions via syncPermissions() (add/remove in single operation)

Response Structure

{
  "data": {
    "id": 1,
    "name": "Regional Manager",
    "description": "Manages multiple branches",
    "permissions": ["employees.read", "shifts.read"],
    "permissions_count": 2,
    "assigned_to": 5,
    "created_at": "2025-11-09T10:00:00.000000Z",
    "updated_at": "2025-11-09T10:00:00.000000Z"
  }
}

🔜 Next Steps

1. Wait for [EPIC] Migrate Permission System from 'web' to 'sanctum' Guard #125 completion - Systematic guard migration via sub-issues
2. All sub-issues completed:
   • Update RoleManagementApiTest to use sanctum guard #126: Update RoleManagementApiTest.php
   • Update RoleApiTest to use sanctum guard #127: Update RoleApiTest.php
   • Update PersonApiTest to use sanctum guard #128: Update PersonApiTest.php
   • Add $guard_name property to User model #129: Add $guard_name to User model
   • Document Guard Architecture and sanctum usage #130: Documentation update
3. Verify all 233 tests pass - Including 36 currently failing
4. Remove DRAFT status - Ready for review
5. Merge - Complete RBAC Phase 4

📊 Current Test Status

Before Guard Migration:

Tests:    36 failed, 197 passed (550 assertions)
- RoleManagementApiTest: 3 failing (guard mismatch)
- RoleApiTest: 27 failing (guard mismatch)
- PersonApiTest: 13 failing (guard mismatch)

Expected After #125:

Tests:    0 failed, 233 passed
- All RBAC tests passing
- Zero technical debt
- Architecturally correct

🔗 Related Issues

Implements:

• RBAC Phase 4: Documentation & Final Testing #108 - RBAC Phase 4: Role Management CRUD API

Blocked by:

• [EPIC] Migrate Permission System from 'web' to 'sanctum' Guard #125 - [EPIC] Migrate Permission System from 'web' to 'sanctum' Guard

📝 Review Notes

This PR demonstrates TDD RED phase intentionally:

• Tests written first (comprehensive coverage)
• Implementation complete and working
• Failures expose architectural issue (not implementation bug)
• Issue documented and tracked via EPIC [EPIC] Migrate Permission System from 'web' to 'sanctum' Guard #125

Architecture Decision: Preferred systematic migration over quick fix to avoid technical debt. This PR preserves context and demonstrates proper issue workflow.

---

Status: 🚧 DRAFT (waiting for #125)
Ready for Review: After #125 completed
Estimated Completion: 2-3 hours after #125 started

[kevalyq]

🎉 Blocker Resolved - Action Required

✅ Issue #125 Complete

The blocking issue (#125 - Guard Migration) has been successfully completed and merged! All 6 sub-issues are done:

• ✅ Update RoleManagementApiTest to use sanctum guard #126, Update RoleApiTest to use sanctum guard #127, Update PersonApiTest to use sanctum guard #128: Test files updated to sanctum guard
• ✅ Add $guard_name property to User model #129: User model updated with $guard_name = 'sanctum'
• ✅ Set sanctum as default guard in config/auth.php #134: Config default guard set to sanctum (via PR fix(auth): Set sanctum as default guard in config/auth.php #135)
• ✅ Document Guard Architecture and sanctum usage #130: Guard architecture documented

⚠️ Merge Conflict Detected

Problem: This PR contains changes to config/auth.php that were already merged to main via PR #135 (Issue #134).

Current state in main:

'defaults' => [
    'guard' => env('AUTH_GUARD', 'sanctum'),  // ✅ Already set
],

'guards' => [
    'sanctum' => [  // ✅ Already configured
        'driver' => 'sanctum',
        'provider' => 'users',
    ],
],

This PR re-adds the same lines → Guaranteed merge conflict.

🔧 Required Actions

1. Rebase on current main branch:

git checkout feat/rbac-phase4-role-crud-api
git fetch origin
git rebase origin/main
# Resolve conflict in config/auth.php by removing duplicate lines
git push --force-with-lease

2. Run full test suite:

ddev exec php artisan test

Expected: All 26 tests in RoleManagementApiTest.php should now pass (guard migration complete).

3. Remove DRAFT status if all tests pass ✅

📋 Follow-up: Separate Topics for Issue #108

According to our "1 Topic = 1 PR" principle, the following features from Issue #108 should become separate PRs:

Feature	Status	Recommendation
Role Management CRUD API	✅ This PR	Correct
Permission CRUD API	❌ Missing	New sub-issue + PR
Direct Permission Assignment	❌ Missing	New sub-issue + PR
Predefined Roles Seeder	❌ Missing	New sub-issue + PR
API Documentation	❌ Missing	New sub-issue + PR

I'll create sub-issues for #108 following the Epic workflow pattern from #125.

🎯 Summary

This PR is technically correct! Just needs:

1. Rebase to resolve config duplicate
2. Test verification (should be green now)
3. DRAFT removal

Once rebased, this should be ready to merge! 🚀

---

References:

• Blocker [EPIC] Migrate Permission System from 'web' to 'sanctum' Guard #125: ✅ CLOSED
• Config Set sanctum as default guard in config/auth.php #134: ✅ MERGED via fix(auth): Set sanctum as default guard in config/auth.php #135
• Parent Epic RBAC Phase 4: Documentation & Final Testing #108: 🚧 Additional sub-issues coming

[kevalyq]

🔍 Test Fix Required: Missing guard_name='sanctum' in Permissions

Problem Identified

The test file RoleManagementApiTest.php creates permissions without explicit guard_name:

// ❌ Current (lines 33-40 in PR)
Permission::create(['name' => 'roles.read']);
Permission::create(['name' => 'roles.create']);
Permission::create(['name' => 'roles.update']);
Permission::create(['name' => 'roles.delete']);

This will cause test failures because:

1. Permissions default to guard_name='web'
2. User authenticated via auth:sanctum middleware
3. Spatie checks for permissions with sanctum guard
4. Guard mismatch → 403 Forbidden

Correct Pattern (from merged #127)

After reviewing RoleApiTest.php (merged via #127), the correct pattern is:

// ✅ Correct (from RoleApiTest.php)
Permission::create(['name' => 'role.assign', 'guard_name' => 'sanctum']);
Permission::create(['name' => 'role.revoke', 'guard_name' => 'sanctum']);
Permission::create(['name' => 'role.read', 'guard_name' => 'sanctum']);

Required Fix

Update tests/Feature/RoleManagementApiTest.php lines 33-40:

// Create permissions for role management
Permission::create(['name' => 'roles.read', 'guard_name' => 'sanctum']);
Permission::create(['name' => 'roles.create', 'guard_name' => 'sanctum']);
Permission::create(['name' => 'roles.update', 'guard_name' => 'sanctum']);
Permission::create(['name' => 'roles.delete', 'guard_name' => 'sanctum']);

// Create test permissions for assignment
Permission::create(['name' => 'employees.read', 'guard_name' => 'sanctum']);
Permission::create(['name' => 'employees.create', 'guard_name' => 'sanctum']);
Permission::create(['name' => 'shifts.read', 'guard_name' => 'sanctum']);

Expected Result

After this fix + rebase:

• ✅ All 26 tests should pass
• ✅ Guard migration [EPIC] Migrate Permission System from 'web' to 'sanctum' Guard #125 complete
• ✅ Ready to remove DRAFT status

---

Next Actions:

1. Rebase PR on main (resolve config/auth.php conflict)
2. Apply test fix (add guard_name='sanctum' to all Permission::create())
3. Run test suite locally
4. Remove DRAFT status if all tests pass

[github-actions]

💡 Tip: Consider Using Draft PRs

Benefits of opening PRs as drafts initially:

• 💰 Saves CI runtime and Copilot review credits
• 🎯 Automatically sets linked issues to "🚧 In Progress" status
• 🚀 Mark "Ready for review" when done to trigger full CI pipeline

How to convert:

1. Click "Still in progress? Convert to draft" in the sidebar, OR
2. Use gh pr ready when ready for review

This is just a friendly reminder - feel free to continue as is! 😊

[Copilot]

Pull Request Overview

This PR implements a comprehensive CRUD API for role management, introducing new endpoints for creating, reading, updating, and deleting roles with permission assignments. The implementation follows Laravel's API resource pattern with proper authorization, validation, and test coverage.

Key changes:

• New RoleManagementController implementing CRUD operations for roles with permission management
• Form request classes (CreateRoleRequest, UpdateRoleRequest) for validation with custom error messages
• RoleManagementPolicy defining authorization rules for role operations
• Comprehensive test suite covering authentication, authorization, validation, and business logic scenarios

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
app/Http/Controllers/Api/V1/RoleManagementController.php	New controller implementing index, store, show, update, and destroy methods for role management
app/Http/Requests/Api/V1/CreateRoleRequest.php	Form request with validation rules for creating roles including permission existence checks
app/Http/Requests/Api/V1/UpdateRoleRequest.php	Form request with validation rules for updating roles with unique name constraint ignoring current role
app/Policies/RoleManagementPolicy.php	Policy class defining viewAny, view, create, update, and delete authorization methods
app/Providers/AppServiceProvider.php	Registers RoleManagementPolicy for Spatie's Role model
routes/api.php	Registers five new role management endpoints under /v1/roles
tests/Feature/RoleManagementApiTest.php	Comprehensive feature tests covering all endpoints with authentication, authorization, and validation scenarios

[github-actions]

💡 Tip: Consider Using Draft PRs

Benefits of opening PRs as drafts initially:

• 💰 Saves CI runtime and Copilot review credits
• 🎯 Automatically sets linked issues to "🚧 In Progress" status
• 🚀 Mark "Ready for review" when done to trigger full CI pipeline

How to convert:

1. Click "Still in progress? Convert to draft" in the sidebar, OR
2. Use gh pr ready when ready for review

This is just a friendly reminder - feel free to continue as is! 😊