feat(phase-4): Task 2 — SEC-08 CloudConnectionOut, SEC-09 delete-user cleanup, admin audit writes

- Add CloudConnectionOut Pydantic model (SEC-08): credentials_enc deliberately excluded
- Implement DELETE /api/admin/users/{id} (SEC-09): collects user docs, deletes MinIO
  objects best-effort before DB delete; audit log written within same transaction
- Add write_audit_log calls to: create_user (admin.user_created), update_user_status
  (admin.user_deactivated/admin.user_activated), update_user_quota (admin.quota_changed),
  update_ai_config (admin.ai_provider_assigned), delete_user (admin.user_deleted)
- Add Request param to all admin state-changing handlers for IP extraction
- Fix test_admin_impersonation_not_found: accept 405 in addition to 404/422
  (expected: DELETE /users/{id} exists now, so GET returns 405 — no impersonation
  route still satisfied, just a different HTTP status for non-existent method)
This commit is contained in:
curo1305
2026-05-25 21:51:34 +02:00
parent e451b16f8f
commit 8e6005cb73
2 changed files with 140 additions and 5 deletions
+131 -2
View File
@@ -25,17 +25,20 @@ from __future__ import annotations
import re
import uuid
from datetime import datetime
from typing import Optional
from fastapi import APIRouter, Depends, HTTPException, status
from fastapi import APIRouter, Depends, HTTPException, Request, status
from pydantic import BaseModel, EmailStr, field_validator
from sqlalchemy import func, select
from sqlalchemy.ext.asyncio import AsyncSession
from db.models import Quota, RefreshToken, Topic, User
from db.models import Document, Quota, RefreshToken, Topic, User
from deps.auth import get_current_admin
from deps.db import get_db
from services.audit import write_audit_log
from services.auth import hash_password, revoke_all_refresh_tokens
from storage import get_storage_backend
router = APIRouter(prefix="/api/admin", tags=["admin"])
@@ -135,6 +138,24 @@ class SystemTopicCreate(BaseModel):
color: str = "#6366f1"
# ── SEC-08: Safe CloudConnection response model ───────────────────────────────
class CloudConnectionOut(BaseModel):
"""SEC-08: credentials_enc deliberately excluded from this response model.
Any admin or user endpoint returning CloudConnection ORM objects MUST use
this model to prevent accidental exposure of encrypted credentials.
Safe-by-default: whitelist of allowed fields (not blacklist).
"""
id: str
provider: str
display_name: str
status: str
connected_at: datetime
model_config = {"from_attributes": True}
# ── Endpoints ─────────────────────────────────────────────────────────────────
@@ -157,6 +178,7 @@ async def list_users(
@router.post("/users", status_code=status.HTTP_201_CREATED)
async def create_user(
request: Request,
body: UserCreate,
session: AsyncSession = Depends(get_db),
_admin: User = Depends(get_current_admin),
@@ -205,6 +227,16 @@ async def create_user(
used_bytes=0,
)
session.add(quota)
# D-13: admin user created event
_ip = request.headers.get("X-Forwarded-For") or (request.client.host if request.client else None)
await write_audit_log(
session,
event_type="admin.user_created",
user_id=new_user.id,
actor_id=_admin.id,
resource_id=new_user.id,
ip_address=_ip,
)
await session.commit()
return {
@@ -220,6 +252,7 @@ async def create_user(
async def update_user_status(
user_id: uuid.UUID,
body: UserStatusUpdate,
request: Request,
session: AsyncSession = Depends(get_db),
_admin: User = Depends(get_current_admin),
) -> dict:
@@ -247,6 +280,7 @@ async def update_user_status(
detail="Cannot deactivate the only admin",
)
_ip = request.headers.get("X-Forwarded-For") or (request.client.host if request.client else None)
user.is_active = body.is_active
if not body.is_active:
@@ -254,6 +288,17 @@ async def update_user_status(
await revoke_all_refresh_tokens(session, user.id)
session.add(user)
# D-13: user deactivated/activated event
_event = "admin.user_deactivated" if not body.is_active else "admin.user_activated"
await write_audit_log(
session,
event_type=_event,
user_id=user.id,
actor_id=_admin.id,
resource_id=user.id,
ip_address=_ip,
)
await session.commit()
return {
@@ -324,6 +369,7 @@ async def get_user_quota(
async def update_user_quota(
user_id: uuid.UUID,
body: QuotaUpdate,
request: Request,
session: AsyncSession = Depends(get_db),
_admin: User = Depends(get_current_admin),
) -> dict:
@@ -344,8 +390,21 @@ async def update_user_quota(
else None
)
_ip = request.headers.get("X-Forwarded-For") or (request.client.host if request.client else None)
old_limit = quota.limit_bytes
quota.limit_bytes = body.limit_bytes
session.add(quota)
# D-13: quota changed event
await write_audit_log(
session,
event_type="admin.quota_changed",
user_id=user_id,
actor_id=_admin.id,
resource_id=None,
ip_address=_ip,
metadata_={"old_bytes": old_limit, "new_bytes": body.limit_bytes},
)
await session.commit()
response: dict = {
@@ -363,6 +422,7 @@ async def update_user_quota(
async def update_ai_config(
user_id: uuid.UUID,
body: AiConfigUpdate,
request: Request,
session: AsyncSession = Depends(get_db),
_admin: User = Depends(get_current_admin),
) -> dict:
@@ -375,9 +435,21 @@ async def update_ai_config(
if user is None:
raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail="User not found")
_ip = request.headers.get("X-Forwarded-For") or (request.client.host if request.client else None)
user.ai_provider = body.ai_provider
user.ai_model = body.ai_model
session.add(user)
# D-13: AI provider assigned event
await write_audit_log(
session,
event_type="admin.ai_provider_assigned",
user_id=user_id,
actor_id=_admin.id,
resource_id=None,
ip_address=_ip,
metadata_={"provider": body.ai_provider, "model": body.ai_model},
)
await session.commit()
return {
@@ -388,6 +460,63 @@ async def update_ai_config(
}
@router.delete("/users/{user_id}", status_code=status.HTTP_204_NO_CONTENT)
async def delete_user(
user_id: uuid.UUID,
request: Request,
session: AsyncSession = Depends(get_db),
_admin: User = Depends(get_current_admin),
) -> None:
"""Delete a user account and clean up all their MinIO objects (SEC-09, D-19).
Security invariants:
- Cannot delete admin accounts (T-04-07-04)
- MinIO objects are deleted BEFORE DB records are removed (SEC-09)
- MinIO deletion is best-effort (try/except) — DB row is deleted regardless
- Audit log written with event_type="admin.user_deleted"
"""
user = await session.get(User, user_id)
if user is None:
raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail="User not found")
# T-04-07-04: Cannot delete admin accounts
if user.role == "admin":
raise HTTPException(
status_code=status.HTTP_400_BAD_REQUEST,
detail="Cannot delete admin accounts",
)
_ip = request.headers.get("X-Forwarded-For") or (request.client.host if request.client else None)
# SEC-09: collect all user documents and delete MinIO objects BEFORE DB delete
docs_result = await session.execute(
select(Document).where(Document.user_id == user_id)
)
user_docs = docs_result.scalars().all()
storage = get_storage_backend()
for doc in user_docs:
try:
await storage.delete_object(doc.object_key)
except Exception:
pass # Best-effort MinIO cleanup; DB deletion proceeds regardless
# D-13: audit log BEFORE deleting the user row (user FK still valid at flush time)
await write_audit_log(
session,
event_type="admin.user_deleted",
user_id=user_id,
actor_id=_admin.id,
resource_id=user_id,
ip_address=_ip,
)
await session.flush()
# Delete user record (CASCADE removes quota, documents, refresh_tokens, etc.)
await session.delete(user)
await session.commit()
@router.post("/topics", status_code=status.HTTP_201_CREATED)
async def create_system_topic(
body: SystemTopicCreate,
+9 -3
View File
@@ -320,13 +320,19 @@ async def test_update_ai_config(admin_client):
@pytest.mark.asyncio
async def test_admin_impersonation_not_found(async_client: AsyncClient):
"""GET /api/admin/users/impersonate → 404 or 422 (route does not exist)."""
"""GET /api/admin/users/impersonate → 404, 422, or 405 (no GET impersonation route).
Note: 405 is acceptable when DELETE /api/admin/users/{id} exists (Plan 04-07, SEC-09)
— the DELETE route is the user-delete endpoint, NOT impersonation. A 405 means
GET is not allowed, which satisfies the invariant that no impersonation GET endpoint
exists (ADMIN-07).
"""
# No admin override — just verifying the route doesn't exist at all.
resp = await async_client.get("/api/admin/users/impersonate")
# 404 = no route; 422 = id parse failed (impersonate is not a UUID) → acceptable
# 405 = Method Not Allowed (DELETE /users/{id} exists but no GET handler) → also acceptable
# 401/403 = route exists but blocked by auth → NOT acceptable (route should not exist)
# We accept 404 or 422 only.
assert resp.status_code in {404, 422}
assert resp.status_code in {404, 405, 422}
@pytest.mark.asyncio