feat: implement audit trail alignment (soft delete & permission audit)

- Task Soft Delete:
  - Add is_deleted, deleted_at, deleted_by fields to Task model
  - Convert DELETE to soft delete with cascade to subtasks
  - Add include_deleted query param (admin only)
  - Add POST /api/tasks/{id}/restore endpoint
  - Exclude deleted tasks from subtask_count

- Permission Change Audit:
  - Add user.role_change event (high sensitivity)
  - Add user.admin_change event (critical, triggers alert)
  - Add PATCH /api/users/{id}/admin endpoint
  - Add role.permission_change event type

- Append-Only Enforcement:
  - Add DB triggers for audit_logs immutability (manual for production)
  - Migration 008 with graceful trigger failure handling

- Tests: 11 new soft delete tests (153 total passing)
- OpenSpec: fix-audit-trail archived, fix-realtime-notifications & fix-weekly-report proposals added

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This commit is contained in:
beabigegg
2025-12-30 06:58:30 +08:00
parent 95c281d8e1
commit 10db2c9d1f
18 changed files with 1455 additions and 12 deletions

View File

@@ -1,4 +1,5 @@
import uuid
from datetime import datetime
from typing import List, Optional
from fastapi import APIRouter, Depends, HTTPException, status, Query, Request
from sqlalchemy.orm import Session
@@ -36,6 +37,11 @@ def get_task_depth(db: Session, task: Task) -> int:
def task_to_response(task: Task) -> TaskWithDetails:
"""Convert a Task model to TaskWithDetails response."""
# Count only non-deleted subtasks
subtask_count = 0
if task.subtasks:
subtask_count = sum(1 for st in task.subtasks if not st.is_deleted)
return TaskWithDetails(
id=task.id,
project_id=task.project_id,
@@ -57,7 +63,7 @@ def task_to_response(task: Task) -> TaskWithDetails:
status_name=task.status.name if task.status else None,
status_color=task.status.color if task.status else None,
creator_name=task.creator.name if task.creator else None,
subtask_count=len(task.subtasks) if task.subtasks else 0,
subtask_count=subtask_count,
)
@@ -67,6 +73,7 @@ async def list_tasks(
parent_task_id: Optional[str] = Query(None, description="Filter by parent task"),
status_id: Optional[str] = Query(None, description="Filter by status"),
assignee_id: Optional[str] = Query(None, description="Filter by assignee"),
include_deleted: bool = Query(False, description="Include deleted tasks (admin only)"),
db: Session = Depends(get_db),
current_user: User = Depends(get_current_user),
):
@@ -89,6 +96,12 @@ async def list_tasks(
query = db.query(Task).filter(Task.project_id == project_id)
# Filter deleted tasks (only admin can include deleted)
if include_deleted and current_user.is_system_admin:
pass # Don't filter by is_deleted
else:
query = query.filter(Task.is_deleted == False)
# Apply filters
if parent_task_id is not None:
if parent_task_id == "":
@@ -238,6 +251,13 @@ async def get_task(
detail="Task not found",
)
# Check if task is deleted (only admin can view deleted)
if task.is_deleted and not current_user.is_system_admin:
raise HTTPException(
status_code=status.HTTP_404_NOT_FOUND,
detail="Task not found",
)
if not check_task_access(current_user, task, task.project):
raise HTTPException(
status_code=status.HTTP_403_FORBIDDEN,
@@ -324,7 +344,7 @@ async def update_task(
return task
@router.delete("/api/tasks/{task_id}", status_code=status.HTTP_204_NO_CONTENT)
@router.delete("/api/tasks/{task_id}", response_model=TaskResponse)
async def delete_task(
task_id: str,
request: Request,
@@ -332,7 +352,7 @@ async def delete_task(
current_user: User = Depends(get_current_user),
):
"""
Delete a task (cascades to subtasks).
Soft delete a task (cascades to subtasks).
"""
task = db.query(Task).filter(Task.id == task_id).first()
@@ -342,13 +362,37 @@ async def delete_task(
detail="Task not found",
)
if task.is_deleted:
raise HTTPException(
status_code=status.HTTP_400_BAD_REQUEST,
detail="Task is already deleted",
)
if not check_task_edit_access(current_user, task, task.project):
raise HTTPException(
status_code=status.HTTP_403_FORBIDDEN,
detail="Permission denied",
)
# Audit log before deletion
now = datetime.utcnow()
# Soft delete the task
task.is_deleted = True
task.deleted_at = now
task.deleted_by = current_user.id
# Cascade soft delete to subtasks
def soft_delete_subtasks(parent_task):
for subtask in parent_task.subtasks:
if not subtask.is_deleted:
subtask.is_deleted = True
subtask.deleted_at = now
subtask.deleted_by = current_user.id
soft_delete_subtasks(subtask)
soft_delete_subtasks(task)
# Audit log
AuditService.log_event(
db=db,
event_type="task.delete",
@@ -356,14 +400,67 @@ async def delete_task(
action=AuditAction.DELETE,
user_id=current_user.id,
resource_id=task.id,
changes=[{"field": "title", "old_value": task.title, "new_value": None}],
changes=[{"field": "is_deleted", "old_value": False, "new_value": True}],
request_metadata=get_audit_metadata(request),
)
db.delete(task)
db.commit()
db.refresh(task)
return None
return task
@router.post("/api/tasks/{task_id}/restore", response_model=TaskResponse)
async def restore_task(
task_id: str,
request: Request,
db: Session = Depends(get_db),
current_user: User = Depends(get_current_user),
):
"""
Restore a soft-deleted task (admin only).
"""
if not current_user.is_system_admin:
raise HTTPException(
status_code=status.HTTP_403_FORBIDDEN,
detail="Only system administrators can restore deleted tasks",
)
task = db.query(Task).filter(Task.id == task_id).first()
if not task:
raise HTTPException(
status_code=status.HTTP_404_NOT_FOUND,
detail="Task not found",
)
if not task.is_deleted:
raise HTTPException(
status_code=status.HTTP_400_BAD_REQUEST,
detail="Task is not deleted",
)
# Restore the task
task.is_deleted = False
task.deleted_at = None
task.deleted_by = None
# Audit log
AuditService.log_event(
db=db,
event_type="task.restore",
resource_type="task",
action=AuditAction.UPDATE,
user_id=current_user.id,
resource_id=task.id,
changes=[{"field": "is_deleted", "old_value": True, "new_value": False}],
request_metadata=get_audit_metadata(request),
)
db.commit()
db.refresh(task)
return task
@router.patch("/api/tasks/{task_id}/status", response_model=TaskResponse)
@@ -495,6 +592,7 @@ async def assign_task(
@router.get("/api/tasks/{task_id}/subtasks", response_model=TaskListResponse)
async def list_subtasks(
task_id: str,
include_deleted: bool = Query(False, description="Include deleted subtasks (admin only)"),
db: Session = Depends(get_db),
current_user: User = Depends(get_current_user),
):
@@ -515,9 +613,13 @@ async def list_subtasks(
detail="Access denied",
)
subtasks = db.query(Task).filter(
Task.parent_task_id == task_id
).order_by(Task.position, Task.created_at).all()
query = db.query(Task).filter(Task.parent_task_id == task_id)
# Filter deleted subtasks (only admin can include deleted)
if not (include_deleted and current_user.is_system_admin):
query = query.filter(Task.is_deleted == False)
subtasks = query.order_by(Task.position, Task.created_at).all()
return TaskListResponse(
tasks=[task_to_response(t) for t in subtasks],

View File

@@ -1,4 +1,4 @@
from fastapi import APIRouter, Depends, HTTPException, status, Query
from fastapi import APIRouter, Depends, HTTPException, status, Query, Request
from sqlalchemy.orm import Session
from sqlalchemy import or_
from typing import List
@@ -6,6 +6,7 @@ from typing import List
from app.core.database import get_db
from app.models.user import User
from app.models.role import Role
from app.models import AuditAction
from app.schemas.user import UserResponse, UserUpdate
from app.middleware.auth import (
get_current_user,
@@ -13,6 +14,8 @@ from app.middleware.auth import (
require_system_admin,
check_department_access,
)
from app.middleware.audit import get_audit_metadata
from app.services.audit_service import AuditService
router = APIRouter()
@@ -135,6 +138,7 @@ async def update_user(
async def assign_role(
user_id: str,
role_id: str,
request: Request,
db: Session = Depends(get_db),
current_user: User = Depends(require_system_admin),
):
@@ -170,7 +174,68 @@ async def assign_role(
detail="Cannot assign system role",
)
old_role_id = user.role_id
user.role_id = role_id
# Audit log for role change (high sensitivity)
if old_role_id != role_id:
AuditService.log_event(
db=db,
event_type="user.role_change",
resource_type="user",
action=AuditAction.UPDATE,
user_id=current_user.id,
resource_id=user.id,
changes=[{"field": "role_id", "old_value": old_role_id, "new_value": role_id}],
request_metadata=get_audit_metadata(request),
)
db.commit()
db.refresh(user)
return user
@router.patch("/{user_id}/admin", response_model=UserResponse)
async def set_admin_status(
user_id: str,
is_admin: bool,
request: Request,
db: Session = Depends(get_db),
current_user: User = Depends(require_system_admin),
):
"""
Set or revoke system administrator status. Requires system admin.
"""
user = db.query(User).filter(User.id == user_id).first()
if not user:
raise HTTPException(
status_code=status.HTTP_404_NOT_FOUND,
detail="User not found",
)
# Prevent self-modification
if user.id == current_user.id:
raise HTTPException(
status_code=status.HTTP_403_FORBIDDEN,
detail="Cannot modify your own admin status",
)
old_admin_status = user.is_system_admin
user.is_system_admin = is_admin
# Audit log for admin change (critical sensitivity, triggers alert)
if old_admin_status != is_admin:
AuditService.log_event(
db=db,
event_type="user.admin_change",
resource_type="user",
action=AuditAction.UPDATE,
user_id=current_user.id,
resource_id=user.id,
changes=[{"field": "is_system_admin", "old_value": old_admin_status, "new_value": is_admin}],
request_metadata=get_audit_metadata(request),
)
db.commit()
db.refresh(user)
return user

View File

@@ -27,6 +27,7 @@ EVENT_SENSITIVITY = {
"task.create": SensitivityLevel.LOW,
"task.update": SensitivityLevel.LOW,
"task.delete": SensitivityLevel.MEDIUM,
"task.restore": SensitivityLevel.MEDIUM,
"task.assign": SensitivityLevel.LOW,
"task.blocker": SensitivityLevel.MEDIUM,
"project.create": SensitivityLevel.MEDIUM,
@@ -34,14 +35,17 @@ EVENT_SENSITIVITY = {
"project.delete": SensitivityLevel.HIGH,
"user.login": SensitivityLevel.LOW,
"user.logout": SensitivityLevel.LOW,
"user.role_change": SensitivityLevel.HIGH,
"user.admin_change": SensitivityLevel.CRITICAL,
"user.permission_change": SensitivityLevel.CRITICAL,
"role.permission_change": SensitivityLevel.CRITICAL,
"attachment.upload": SensitivityLevel.LOW,
"attachment.download": SensitivityLevel.LOW,
"attachment.delete": SensitivityLevel.MEDIUM,
}
# Events that should trigger alerts
ALERT_EVENTS = {"project.delete", "user.permission_change"}
ALERT_EVENTS = {"project.delete", "user.permission_change", "user.admin_change", "role.permission_change"}
class AuditLog(Base):

View File

@@ -36,12 +36,18 @@ class Task(Base):
created_at = Column(DateTime, server_default=func.now(), nullable=False)
updated_at = Column(DateTime, server_default=func.now(), onupdate=func.now(), nullable=False)
# Soft delete fields
is_deleted = Column(Boolean, default=False, nullable=False, index=True)
deleted_at = Column(DateTime, nullable=True)
deleted_by = Column(String(36), ForeignKey("pjctrl_users.id"), nullable=True)
# Relationships
project = relationship("Project", back_populates="tasks")
parent_task = relationship("Task", remote_side=[id], back_populates="subtasks")
subtasks = relationship("Task", back_populates="parent_task", cascade="all, delete-orphan")
assignee = relationship("User", foreign_keys=[assignee_id], back_populates="assigned_tasks")
creator = relationship("User", foreign_keys=[created_by], back_populates="created_tasks")
deleter = relationship("User", foreign_keys=[deleted_by])
status = relationship("TaskStatus", back_populates="tasks")
# Collaboration relationships

View File

@@ -0,0 +1,98 @@
"""Task soft delete fields and audit log immutability
Revision ID: 008
Revises: 007
Create Date: 2024-12-30
"""
from alembic import op
import sqlalchemy as sa
import logging
# revision identifiers, used by Alembic.
revision = '008'
down_revision = '007'
branch_labels = None
depends_on = None
logger = logging.getLogger(__name__)
def upgrade():
# Add soft delete fields to tasks
op.add_column('pjctrl_tasks', sa.Column('is_deleted', sa.Boolean(), nullable=False, server_default='0'))
op.add_column('pjctrl_tasks', sa.Column('deleted_at', sa.DateTime(), nullable=True))
op.add_column('pjctrl_tasks', sa.Column('deleted_by', sa.String(36), nullable=True))
# Add foreign key for deleted_by
op.create_foreign_key(
'fk_tasks_deleted_by_users',
'pjctrl_tasks', 'pjctrl_users',
['deleted_by'], ['id']
)
# Add index for soft delete filtering
op.create_index('idx_task_deleted', 'pjctrl_tasks', ['is_deleted'])
# Create append-only triggers for audit_logs table
# Note: These triggers require SUPER privilege in MySQL with binary logging
# For production, run these manually with appropriate privileges:
#
# CREATE TRIGGER prevent_audit_update
# BEFORE UPDATE ON pjctrl_audit_logs
# FOR EACH ROW
# BEGIN
# SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'Audit logs are immutable';
# END;
#
# CREATE TRIGGER prevent_audit_delete
# BEFORE DELETE ON pjctrl_audit_logs
# FOR EACH ROW
# BEGIN
# SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'Audit logs are immutable';
# END;
try:
# Prevent UPDATE on audit_logs
op.execute("""
CREATE TRIGGER prevent_audit_update
BEFORE UPDATE ON pjctrl_audit_logs
FOR EACH ROW
BEGIN
SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'Audit logs are immutable and cannot be updated';
END
""")
# Prevent DELETE on audit_logs
op.execute("""
CREATE TRIGGER prevent_audit_delete
BEFORE DELETE ON pjctrl_audit_logs
FOR EACH ROW
BEGIN
SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'Audit logs are immutable and cannot be deleted';
END
""")
except Exception as e:
# Triggers may fail due to privilege issues or SQLite (in tests)
logger.warning(f"Could not create audit immutability triggers: {e}")
logger.warning("For production, create triggers manually with SUPER privilege")
def downgrade():
# Remove triggers (silently ignore if they don't exist)
try:
op.execute("DROP TRIGGER IF EXISTS prevent_audit_update")
op.execute("DROP TRIGGER IF EXISTS prevent_audit_delete")
except Exception:
pass
# Remove index
op.drop_index('idx_task_deleted', 'pjctrl_tasks')
# Remove foreign key
op.drop_constraint('fk_tasks_deleted_by_users', 'pjctrl_tasks', type_='foreignkey')
# Remove columns
op.drop_column('pjctrl_tasks', 'deleted_by')
op.drop_column('pjctrl_tasks', 'deleted_at')
op.drop_column('pjctrl_tasks', 'is_deleted')

View File

@@ -0,0 +1,347 @@
import pytest
import uuid
from app.models import User, Space, Project, Task, TaskStatus
@pytest.fixture
def test_admin(db):
"""Create a test admin user."""
user = User(
id=str(uuid.uuid4()),
email="admin@example.com",
name="Admin User",
role_id="00000000-0000-0000-0000-000000000001",
is_active=True,
is_system_admin=True,
)
db.add(user)
db.commit()
return user
@pytest.fixture
def test_regular_user(db):
"""Create a test regular user."""
user = User(
id=str(uuid.uuid4()),
email="regular@example.com",
name="Regular User",
role_id="00000000-0000-0000-0000-000000000003",
is_active=True,
is_system_admin=False,
)
db.add(user)
db.commit()
return user
@pytest.fixture
def admin_token(client, mock_redis, test_admin):
"""Get a token for admin user."""
from app.core.security import create_access_token, create_token_payload
token_data = create_token_payload(
user_id=test_admin.id,
email=test_admin.email,
role="super_admin",
department_id=None,
is_system_admin=True,
)
token = create_access_token(token_data)
mock_redis.setex(f"session:{test_admin.id}", 900, token)
return token
@pytest.fixture
def regular_token(client, mock_redis, test_regular_user):
"""Get a token for regular user."""
from app.core.security import create_access_token, create_token_payload
token_data = create_token_payload(
user_id=test_regular_user.id,
email=test_regular_user.email,
role="engineer",
department_id=None,
is_system_admin=False,
)
token = create_access_token(token_data)
mock_redis.setex(f"session:{test_regular_user.id}", 900, token)
return token
@pytest.fixture
def test_space(db, test_admin):
"""Create a test space."""
space = Space(
id=str(uuid.uuid4()),
name="Test Space",
description="Test space",
owner_id=test_admin.id,
)
db.add(space)
db.commit()
return space
@pytest.fixture
def test_project(db, test_space, test_admin):
"""Create a test project with public access."""
project = Project(
id=str(uuid.uuid4()),
space_id=test_space.id,
title="Test Project",
description="Test project",
owner_id=test_admin.id,
security_level="public", # Allow all users to access
)
db.add(project)
db.commit()
return project
@pytest.fixture
def test_status(db, test_project):
"""Create a test task status."""
status = TaskStatus(
id=str(uuid.uuid4()),
project_id=test_project.id,
name="To Do",
color="#808080",
position=0,
)
db.add(status)
db.commit()
return status
@pytest.fixture
def test_task(db, test_project, test_admin, test_status):
"""Create a test task."""
task = Task(
id=str(uuid.uuid4()),
project_id=test_project.id,
title="Test Task",
status_id=test_status.id,
created_by=test_admin.id,
)
db.add(task)
db.commit()
return task
@pytest.fixture
def test_task_with_subtask(db, test_project, test_admin, test_status, test_task):
"""Create a test task with subtask."""
subtask = Task(
id=str(uuid.uuid4()),
project_id=test_project.id,
parent_task_id=test_task.id,
title="Subtask",
status_id=test_status.id,
created_by=test_admin.id,
)
db.add(subtask)
db.commit()
return subtask
class TestSoftDelete:
"""Tests for soft delete functionality."""
def test_delete_task_soft_deletes(self, client, admin_token, test_task, db):
"""Test that DELETE soft-deletes a task."""
response = client.delete(
f"/api/tasks/{test_task.id}",
headers={"Authorization": f"Bearer {admin_token}"},
)
assert response.status_code == 200
data = response.json()
assert data["id"] == test_task.id
# Verify in database
db.refresh(test_task)
assert test_task.is_deleted is True
assert test_task.deleted_at is not None
assert test_task.deleted_by is not None
def test_deleted_task_not_in_list(self, client, admin_token, test_project, test_task, db):
"""Test that deleted tasks are not shown in list."""
# Delete the task
client.delete(
f"/api/tasks/{test_task.id}",
headers={"Authorization": f"Bearer {admin_token}"},
)
# List tasks
response = client.get(
f"/api/projects/{test_project.id}/tasks",
headers={"Authorization": f"Bearer {admin_token}"},
)
assert response.status_code == 200
data = response.json()
assert data["total"] == 0
def test_admin_can_list_deleted_with_include_deleted(self, client, admin_token, test_project, test_task, db):
"""Test that admin can see deleted tasks with include_deleted parameter."""
# Delete the task
client.delete(
f"/api/tasks/{test_task.id}",
headers={"Authorization": f"Bearer {admin_token}"},
)
# List with include_deleted
response = client.get(
f"/api/projects/{test_project.id}/tasks?include_deleted=true",
headers={"Authorization": f"Bearer {admin_token}"},
)
assert response.status_code == 200
data = response.json()
assert data["total"] == 1
assert data["tasks"][0]["id"] == test_task.id
def test_regular_user_cannot_see_deleted_with_include_deleted(self, client, regular_token, test_project, test_task, admin_token, db):
"""Test that non-admin cannot see deleted tasks even with include_deleted."""
# Delete the task as admin
client.delete(
f"/api/tasks/{test_task.id}",
headers={"Authorization": f"Bearer {admin_token}"},
)
# Try to list with include_deleted as regular user
response = client.get(
f"/api/projects/{test_project.id}/tasks?include_deleted=true",
headers={"Authorization": f"Bearer {regular_token}"},
)
assert response.status_code == 200
data = response.json()
assert data["total"] == 0
def test_get_deleted_task_returns_404_for_regular_user(self, client, admin_token, regular_token, test_task, db):
"""Test that getting a deleted task returns 404 for non-admin."""
# Delete the task
client.delete(
f"/api/tasks/{test_task.id}",
headers={"Authorization": f"Bearer {admin_token}"},
)
# Try to get as regular user
response = client.get(
f"/api/tasks/{test_task.id}",
headers={"Authorization": f"Bearer {regular_token}"},
)
assert response.status_code == 404
def test_admin_can_view_deleted_task(self, client, admin_token, test_task, db):
"""Test that admin can view a deleted task."""
# Delete the task
client.delete(
f"/api/tasks/{test_task.id}",
headers={"Authorization": f"Bearer {admin_token}"},
)
# Get as admin
response = client.get(
f"/api/tasks/{test_task.id}",
headers={"Authorization": f"Bearer {admin_token}"},
)
assert response.status_code == 200
def test_cascade_soft_delete_subtasks(self, client, admin_token, test_task, test_task_with_subtask, db):
"""Test that deleting a parent task soft-deletes its subtasks."""
# Delete the parent task
response = client.delete(
f"/api/tasks/{test_task.id}",
headers={"Authorization": f"Bearer {admin_token}"},
)
assert response.status_code == 200
# Verify subtask is also soft-deleted
db.refresh(test_task_with_subtask)
assert test_task_with_subtask.is_deleted is True
class TestRestoreTask:
"""Tests for task restoration functionality."""
def test_restore_task(self, client, admin_token, test_task, db):
"""Test that admin can restore a deleted task."""
# Delete the task
client.delete(
f"/api/tasks/{test_task.id}",
headers={"Authorization": f"Bearer {admin_token}"},
)
# Restore the task
response = client.post(
f"/api/tasks/{test_task.id}/restore",
headers={"Authorization": f"Bearer {admin_token}"},
)
assert response.status_code == 200
# Verify in database
db.refresh(test_task)
assert test_task.is_deleted is False
assert test_task.deleted_at is None
assert test_task.deleted_by is None
def test_regular_user_cannot_restore(self, client, admin_token, regular_token, test_task, db):
"""Test that non-admin cannot restore a deleted task."""
# Delete the task
client.delete(
f"/api/tasks/{test_task.id}",
headers={"Authorization": f"Bearer {admin_token}"},
)
# Try to restore as regular user
response = client.post(
f"/api/tasks/{test_task.id}/restore",
headers={"Authorization": f"Bearer {regular_token}"},
)
assert response.status_code == 403
def test_cannot_restore_non_deleted_task(self, client, admin_token, test_task, db):
"""Test that restoring a non-deleted task returns error."""
response = client.post(
f"/api/tasks/{test_task.id}/restore",
headers={"Authorization": f"Bearer {admin_token}"},
)
assert response.status_code == 400
assert "not deleted" in response.json()["detail"]
class TestSubtaskCount:
"""Tests for subtask count excluding deleted."""
def test_subtask_count_excludes_deleted(self, client, admin_token, test_task, test_task_with_subtask, db):
"""Test that subtask_count excludes deleted subtasks."""
# Get parent task before deletion
response = client.get(
f"/api/tasks/{test_task.id}",
headers={"Authorization": f"Bearer {admin_token}"},
)
assert response.status_code == 200
assert response.json()["subtask_count"] == 1
# Delete subtask
client.delete(
f"/api/tasks/{test_task_with_subtask.id}",
headers={"Authorization": f"Bearer {admin_token}"},
)
# Get parent task after deletion
response = client.get(
f"/api/tasks/{test_task.id}",
headers={"Authorization": f"Bearer {admin_token}"},
)
assert response.status_code == 200
assert response.json()["subtask_count"] == 0