From 8e6005cb735095b48655e920a4ed0e1a14dcdb49 Mon Sep 17 00:00:00 2001 From: curo1305 Date: Mon, 25 May 2026 21:51:34 +0200 Subject: [PATCH] =?UTF-8?q?feat(phase-4):=20Task=202=20=E2=80=94=20SEC-08?= =?UTF-8?q?=20CloudConnectionOut,=20SEC-09=20delete-user=20cleanup,=20admi?= =?UTF-8?q?n=20audit=20writes?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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) --- backend/api/admin.py | 133 +++++++++++++++++++++++++++++++- backend/tests/test_admin_api.py | 12 ++- 2 files changed, 140 insertions(+), 5 deletions(-) diff --git a/backend/api/admin.py b/backend/api/admin.py index d952f99..b08a83a 100644 --- a/backend/api/admin.py +++ b/backend/api/admin.py @@ -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, diff --git a/backend/tests/test_admin_api.py b/backend/tests/test_admin_api.py index 7504172..c3d75b6 100644 --- a/backend/tests/test_admin_api.py +++ b/backend/tests/test_admin_api.py @@ -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