Skip to content

Conversation

@kevalyq
Copy link
Contributor

@kevalyq kevalyq commented Nov 8, 2025

Summary

Implements RBAC Phase 2: Temporal Logic & Auto-Expiration with automatic expiration of time-limited role assignments, comprehensive audit trail, and scheduled execution.

Changes

Core Features

  • roles:expire command runs every minute via Laravel scheduler
  • ✅ Only processes roles with auto_revoke=true flag
  • ✅ Logs all expirations to role_assignments_log before deletion
  • ✅ Transaction-based processing ensures data integrity
  • ✅ Timezone-aware (UTC storage)

Implementation Details

  • Command: app/Console/Commands/ExpireRoles.php
  • Scheduler: routes/console.php (everyMinute)
  • Query-based deletion: Pivot table has no primary key
  • Scope filtering: Uses TemporalRoleUser::expired() scope

Bug Fixes

  • Migration: assigned_by changed from uuid() to foreignId() (matches User table)
  • TemporalRoleUser: Added PHPDoc for pivot properties (PHPStan compliance)
  • Migration: Fixed .after() method placement for foreign key constraint

Tests

  • 10 new tests (31 assertions) in ExpireRolesCommandTest
  • Covers: expiration logic, audit logging, auto_revoke flag
  • Tests: active/future role preservation, timezone handling
  • Validates: batch processing, mixed scenarios

Quality Gates

  • All 179 tests pass (513 assertions)
  • PHPStan Level 9: No errors
  • Laravel Pint: Code style compliant
  • REUSE 3.3: Compliant
  • Pre-push checks: All passed (350 changed lines)

Database Changes

Migration Required: Yes

php artisan migrate

Scheduler Setup: Required (if not already configured)

* * * * * cd /path/to/project && php artisan schedule:run >> /dev/null 2>&1

Related

Breaking Changes

None

Checklist

  • Tests written (TDD approach)
  • All tests pass
  • PHPStan Level 9 passes
  • Laravel Pint passes
  • REUSE 3.3 compliant
  • CHANGELOG.md updated
  • Migration tested (fresh + rollback)
  • Scheduler command tested manually
  • Documentation inline (PHPDoc)
  • No security issues
  • No performance concerns

Implements automatic expiration of time-limited role assignments with
comprehensive audit trail and scheduled execution.

**Core Features:**
- roles:expire command runs every minute via Laravel scheduler
- Only processes roles with auto_revoke=true flag
- Logs all expirations to role_assignments_log before deletion
- Transaction-based processing ensures data integrity
- Timezone-aware (UTC storage)

**Implementation:**
- ExpireRoles command in app/Console/Commands/
- Scheduler config in routes/console.php (everyMinute)
- Query-based deletion (pivot table has no primary key)
- Uses TemporalRoleUser::expired() scope for filtering

**Tests:**
- 10 comprehensive tests (31 assertions)
- Covers: expiration logic, audit logging, auto_revoke flag
- Tests: active/future role preservation, timezone handling
- Validates: batch processing, mixed scenarios

**Bug Fixes:**
- Migration: assigned_by changed from uuid() to foreignId()
- TemporalRoleUser: Added PHPDoc for pivot properties
- PHPStan Level 9 compliance

**Quality Gates:**
✅ All 179 tests pass (513 assertions)
✅ PHPStan Level 9: No errors
✅ Laravel Pint: Code style compliant
✅ REUSE 3.3: Compliant

**Related:**
- Issue: #106 (RBAC Phase 2)
- Epic: #5 (RBAC System)
- ADR: ADR-004 (RBAC Architecture)
- Depends on: Phase 1 (merged)
- Blocks: Phase 3 (#107) - API Endpoints

**Breaking Changes:** None

**Migration Required:** Yes
- run: php artisan migrate
- adds scheduler requirement: * * * * * cd /path && php artisan schedule:run
Copilot AI review requested due to automatic review settings November 8, 2025 22:44
@github-actions
Copy link

github-actions bot commented Nov 8, 2025

💡 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! 😊

@kevalyq kevalyq merged commit 74108b2 into main Nov 8, 2025
23 checks passed
@kevalyq kevalyq deleted the feature/rbac-phase2-temporal-expiration branch November 8, 2025 22:45
@github-actions
Copy link

github-actions bot commented Nov 8, 2025

💡 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! 😊

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR implements automatic expiration of time-limited role assignments (RBAC Phase 2). A scheduled console command roles:expire runs every minute to identify and revoke expired roles where auto_revoke=true, logging each expiration to an immutable audit trail before deletion. The PR also includes a database schema fix, changing the assigned_by column from UUID to bigInteger to properly reference the users table.

Key Changes:

  • New roles:expire command scheduled to run every minute via Laravel scheduler
  • Transaction-based processing ensures audit logs are created before role deletion
  • Comprehensive test suite with 10 tests covering expiration logic, audit logging, timezone handling, and batch processing

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
app/Console/Commands/ExpireRoles.php New scheduled command to expire and revoke temporal role assignments with auto_revoke flag
tests/Feature/Console/ExpireRolesCommandTest.php Comprehensive test suite with 10 tests covering various expiration scenarios
routes/console.php Scheduler configuration to run roles:expire command every minute
database/migrations/2025_11_08_143609_add_temporal_columns_to_model_has_roles_table.php Fixed assigned_by column type from UUID to foreignId (bigInteger) to match users table
app/Models/TemporalRoleUser.php Updated PHPDoc to reflect assigned_by as int instead of string
CHANGELOG.md Documented RBAC Phase 2 temporal logic and auto-expiration feature

Comment on lines +9 to +10
declare(strict_types=1);

Copy link

Copilot AI Nov 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The declare(strict_types=1); directive is inconsistent with other console commands in the codebase. Other commands like GenerateTenantCommand, RotateDekCommand, and RotateKekCommand don't include this directive. For consistency, consider removing it to match the existing command pattern.

Suggested change
declare(strict_types=1);

Copilot uses AI. Check for mistakes.
Comment on lines +53 to +76
foreach ($expired as $assignment) {
DB::transaction(function () use ($assignment, &$count) {
// 1. Log to immutable audit trail
RoleAssignmentLog::create([
'user_id' => $assignment->model_id,
'role_id' => $assignment->role_id,
'action' => 'expired',
'valid_from' => $assignment->valid_from,
'valid_until' => $assignment->valid_until,
'assigned_by' => $assignment->assigned_by,
'reason' => $assignment->reason,
]);

// 2. Delete expired assignment using query (pivot has no primary key)
DB::table('model_has_roles')
->where('model_type', $assignment->model_type)
->where('model_id', $assignment->model_id)
->where('role_id', $assignment->role_id)
->where('tenant_id', $assignment->tenant_id)
->delete();

$count++;
});
}
Copy link

Copilot AI Nov 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using a separate transaction for each expired role could be inefficient when processing many expired roles. Consider wrapping the entire foreach loop in a single transaction, or batch process the deletions. This would improve performance and reduce database overhead when multiple roles expire simultaneously.

DB::transaction(function () use ($expired, &$count) {
    foreach ($expired as $assignment) {
        // Log and delete operations here
        $count++;
    }
});

Copilot uses AI. Check for mistakes.
Comment on lines +41 to +75
// Find expired roles using the expired() scope
$expired = TemporalRoleUser::expired()->get();

if ($expired->isEmpty()) {
$this->info('No expired roles found.');

return self::SUCCESS;
}

$count = 0;

// Process each expired assignment in a transaction for safety
foreach ($expired as $assignment) {
DB::transaction(function () use ($assignment, &$count) {
// 1. Log to immutable audit trail
RoleAssignmentLog::create([
'user_id' => $assignment->model_id,
'role_id' => $assignment->role_id,
'action' => 'expired',
'valid_from' => $assignment->valid_from,
'valid_until' => $assignment->valid_until,
'assigned_by' => $assignment->assigned_by,
'reason' => $assignment->reason,
]);

// 2. Delete expired assignment using query (pivot has no primary key)
DB::table('model_has_roles')
->where('model_type', $assignment->model_type)
->where('model_id', $assignment->model_id)
->where('role_id', $assignment->role_id)
->where('tenant_id', $assignment->tenant_id)
->delete();

$count++;
});
Copy link

Copilot AI Nov 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Loading all expired roles into memory with get() could cause memory issues if there are many expired roles to process. Consider using chunk() or cursor() to process expired roles in batches:

TemporalRoleUser::expired()->chunk(100, function ($expired) use (&$count) {
    foreach ($expired as $assignment) {
        DB::transaction(function () use ($assignment, &$count) {
            // ... existing logic
        });
    }
});
Suggested change
// Find expired roles using the expired() scope
$expired = TemporalRoleUser::expired()->get();
if ($expired->isEmpty()) {
$this->info('No expired roles found.');
return self::SUCCESS;
}
$count = 0;
// Process each expired assignment in a transaction for safety
foreach ($expired as $assignment) {
DB::transaction(function () use ($assignment, &$count) {
// 1. Log to immutable audit trail
RoleAssignmentLog::create([
'user_id' => $assignment->model_id,
'role_id' => $assignment->role_id,
'action' => 'expired',
'valid_from' => $assignment->valid_from,
'valid_until' => $assignment->valid_until,
'assigned_by' => $assignment->assigned_by,
'reason' => $assignment->reason,
]);
// 2. Delete expired assignment using query (pivot has no primary key)
DB::table('model_has_roles')
->where('model_type', $assignment->model_type)
->where('model_id', $assignment->model_id)
->where('role_id', $assignment->role_id)
->where('tenant_id', $assignment->tenant_id)
->delete();
$count++;
});
// Process expired roles in batches to avoid memory issues
$count = 0;
$found = false;
TemporalRoleUser::expired()->chunk(100, function ($expired) use (&$count, &$found) {
$found = true;
foreach ($expired as $assignment) {
DB::transaction(function () use ($assignment, &$count) {
// 1. Log to immutable audit trail
RoleAssignmentLog::create([
'user_id' => $assignment->model_id,
'role_id' => $assignment->role_id,
'action' => 'expired',
'valid_from' => $assignment->valid_from,
'valid_until' => $assignment->valid_until,
'assigned_by' => $assignment->assigned_by,
'reason' => $assignment->reason,
]);
// 2. Delete expired assignment using query (pivot has no primary key)
DB::table('model_has_roles')
->where('model_type', $assignment->model_type)
->where('model_id', $assignment->model_id)
->where('role_id', $assignment->role_id)
->where('tenant_id', $assignment->tenant_id)
->delete();
$count++;
});
}
});
if (! $found) {
$this->info('No expired roles found.');
return self::SUCCESS;

Copilot uses AI. Check for mistakes.
Comment on lines +55 to +74
// 1. Log to immutable audit trail
RoleAssignmentLog::create([
'user_id' => $assignment->model_id,
'role_id' => $assignment->role_id,
'action' => 'expired',
'valid_from' => $assignment->valid_from,
'valid_until' => $assignment->valid_until,
'assigned_by' => $assignment->assigned_by,
'reason' => $assignment->reason,
]);

// 2. Delete expired assignment using query (pivot has no primary key)
DB::table('model_has_roles')
->where('model_type', $assignment->model_type)
->where('model_id', $assignment->model_id)
->where('role_id', $assignment->role_id)
->where('tenant_id', $assignment->tenant_id)
->delete();

$count++;
Copy link

Copilot AI Nov 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a potential race condition if multiple scheduler instances run simultaneously (though Laravel's scheduler should prevent this). If two processes fetch the same expired roles, both will create audit log entries before deletion, resulting in duplicate audit logs.

Consider using a database lock or checking the delete result:

DB::transaction(function () use ($assignment, &$count) {
    $deleted = DB::table('model_has_roles')
        ->where('model_type', $assignment->model_type)
        ->where('model_id', $assignment->model_id)
        ->where('role_id', $assignment->role_id)
        ->where('tenant_id', $assignment->tenant_id)
        ->delete();
    
    // Only log if we actually deleted something
    if ($deleted > 0) {
        RoleAssignmentLog::create([...]);
        $count++;
    }
});

Or reverse the order and use lockForUpdate() on the query.

Suggested change
// 1. Log to immutable audit trail
RoleAssignmentLog::create([
'user_id' => $assignment->model_id,
'role_id' => $assignment->role_id,
'action' => 'expired',
'valid_from' => $assignment->valid_from,
'valid_until' => $assignment->valid_until,
'assigned_by' => $assignment->assigned_by,
'reason' => $assignment->reason,
]);
// 2. Delete expired assignment using query (pivot has no primary key)
DB::table('model_has_roles')
->where('model_type', $assignment->model_type)
->where('model_id', $assignment->model_id)
->where('role_id', $assignment->role_id)
->where('tenant_id', $assignment->tenant_id)
->delete();
$count++;
// 1. Delete expired assignment using query (pivot has no primary key)
$deleted = DB::table('model_has_roles')
->where('model_type', $assignment->model_type)
->where('model_id', $assignment->model_id)
->where('role_id', $assignment->role_id)
->where('tenant_id', $assignment->tenant_id)
->delete();
// 2. Only log to immutable audit trail if we actually deleted something
if ($deleted > 0) {
RoleAssignmentLog::create([
'user_id' => $assignment->model_id,
'role_id' => $assignment->role_id,
'action' => 'expired',
'valid_from' => $assignment->valid_from,
'valid_until' => $assignment->valid_until,
'assigned_by' => $assignment->assigned_by,
'reason' => $assignment->reason,
]);
$count++;
}

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants