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:
+131
-2
@@ -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,
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user