mirror of
https://github.com/bytedance/deer-flow.git
synced 2026-10-26 21:26:39 +00:00
* feat(authz): account disable/enable across every authentication surface (#4063 gap 3) Vertical slice 1 of the user-lifecycle work: an operator can suspend an account and every authentication surface rejects it, while the row — credentials, role, OAuth linkage — is retained so re-enabling restores exactly what was suspended. - users.disabled (NOT NULL, server default 0) via migration 0036_user_disabled; User/UserResponse carry and expose it. - Enforcement at all four surfaces: password login (no credential comparison happens), JWT session resolution (401 ACCOUNT_DISABLED — sessions die at the next request), PAT resolution (the generic 401, no oracle), OAuth provisioning (403 with a contact-admin message; the link is kept so re-enable restores access). - SQLiteUserRepository.set_disabled: single-column, serialized write (same idiom as update_system_role) with the last-ACTIVE-admin guard — disabled admins cannot authenticate, so they cannot restore management access; re-enabling is always allowed. Field-scoping completed both ways: the credential writer mirrors and never writes disabled, so a stale password-change snapshot cannot re-enable a suspended account. - PATCH /api/v1/admin/users/{id} becomes a partial account update (system_role and/or disabled), each field routed to its field-scoped writer; empty bodies 422. Tests 11->18: route (disable/re-enable, last-active-admin 409, empty 422, combined update), real-repo (field isolation both directions, disabled-admin-not-active, guard inside the serialized write, re-enable always), login rejection with the CORRECT password (only the disabled gate can reject) plus resolver-visible flag. Mutations verified red: guard removed (M19), login gate removed (M20 — after hardening the test with a real hash so the gate is load-bearing). * fix(authz): portable boolean default, alternate-authenticator suspension, active-admin role guard Review round 1 on #6566 — three P1s: - [PG default] sa.text("0") is a raw integer expression PostgreSQL rejects as a BOOLEAN default, breaking startup migration and fresh create_all bootstraps alike. Both the migration and the ORM model now use sa.false()/false() — dialect-rendered, portable. - [alternate authenticators] WebSockets bypass AuthMiddleware, so a disabled user's still-valid cookie could open a NEW browser/stream connection and drive the retained session; the standalone LangGraph authenticate callback accepted the same suspended JWT. Both now route through a shared post-lookup validator (validate_resolved_session_user in deps.py — token_version + disabled in one place) that the HTTP dependency also uses, so a lifecycle change lands everywhere at once. - [active-admin role guard] update_system_role counted every admin row, so 'disable B, then demote self' — or a combined {system_role, disabled} self-update — could strand the deployment with zero administrators able to authenticate or re-enable anyone. The demotion guard now counts ACTIVE admins only (disabled admins cannot authenticate), under the same serialization boundary. Tests 18->22: shared-validator verdicts, WS admission after suspension (real token, real repo), LangGraph-callback admission after suspension, disable-then-demote-self + combined self-update both rejected (fake-repo route level and real-repo update_system_role level). Mutations verified red: helper's disabled check removed (M21 — three admission tests fail), active-admin predicate reverted to all-admins (M22 — repo-level strand scenario fails). 366 tests green on the affected surface. * fix(migrations): chain 0036_user_disabled after upstream 0037 The maintainer main-merge brought 0037_project_document_summaries, which branched from 0035 independently of this PR's 0036 — alembic now sees two heads and every engine bootstrap fails. Chain 0036 after 0037 (single head; revision numbers are labels, the graph is what matters). * fix(migrations): renumber user_disabled to 0038 and follow the chain-test convention Upstream landed its own 0036 (run_idempotency_request) with 0037 chained after it while this PR was in review — my 0036_user_disabled collided on the number and my interim chain-fix (0036 revises 0037) left the graph in a state upstream's chain tests reject (heads == [0037] hardcoded). - 0036_user_disabled -> 0038_user_disabled, revises 0037_project_document_summaries: single head 0038. - Chain tests follow the repo convention (each migration PR appends the new head): 0032/0035 gain the DOC_SUMMARIES link and CURRENT_HEAD = 0038; 0037's single-head assertion moves to CURRENT_HEAD with its own-revision link check unchanged. * fix(migrations): pin the remaining chain tests to the 0038_user_disabled head The 0027/0028, 0029 and 0034 chain tests still pinned the single head to 0037_project_document_summaries (and 0034 asserted the head's parent was 0036_run_idempotency_request), so the renumbered lifecycle migration broke two unit-test shards. Follow the chain-test convention: every pinned head moves to 0038_user_disabled and 0034 walks the chain through 0037. * fix(authz): enforce account suspension on the internal-auth dispatch surface Round-3 review finding: the internal-auth principal is synthesized from trusted headers without a users-row lookup, so a suspended user with an active channel binding kept driving the agent, and scheduler-owned runs (their fabricated in-process requests skip AuthMiddleware entirely) kept launching. Two gates now enforce the lifecycle there: - AuthMiddleware rejects owner-bound internal calls whose users row is disabled (401 account_disabled) at the single gate every internal HTTP call passes through. Auth-disabled mode is exempt (no suspendable accounts, and its owner header carries the synthetic identity), and owner ids without a users row stay allowed — a nonexistent account cannot be suspended. - start_run re-asserts it at run admission after the owner is resolved for attribution: the scheduler records the rejection as the occurrence's launch error and no run record is created. Also addresses the remaining round-3 nits: drop the inline token_version check duplicated by validate_resolved_session_user, correct the local_provider comment (login never surfaces ACCOUNT_DISABLED), refresh the stale 0038 docstring header, and restore the deps module caches in the two suspension tests instead of leaking a closed engine. * fix(auth): carry the disabled field on the frontend synthetic users UserResponse now always includes disabled, so the auth-disabled contract e2e (gateway /auth/me deep-equals the frontend synthetic user) failed without the field on AUTH_DISABLED_USER. Add disabled to both synthetic users and declare it optional on the zod schema — absent still parses as not disabled for pre-gap-3 backends. * fix(authz): log when the internal-owner suspension check is skipped The middleware gate's users-lookup failure path silently degraded to allow; keep the fail-open behavior but leave a WARNING with the exception, so a store that cannot be read is observable in production logs instead of invisible. --------- Co-authored-by: Willem Jiang <willem.jiang@gmail.com>
120 lines
5.5 KiB
Python
120 lines
5.5 KiB
Python
"""Admin user-management surface: list users and assign system roles.
|
|
|
|
RFC #4063 / issue #3462 gap 2: the RBAC provider accepts arbitrary configured
|
|
role names (``guest``, ...), but no surface could assign them to real users —
|
|
``User.system_role`` was pinned to admin/user at the API layer while the DB
|
|
column stayed a plain string for exactly this future. This router is that
|
|
surface: admin-only (§12 Q6 — management surfaces stay admin-gated), role
|
|
names validated against the provider's configured roles, and the last
|
|
remaining admin cannot be demoted (bootstrap lockout protection).
|
|
|
|
Enforcement is unchanged: the assignment only changes which role the
|
|
principal carries; the Gateway's ``@require_permission`` route guards and the
|
|
runtime provider decisions remain the single enforcement points.
|
|
"""
|
|
|
|
from __future__ import annotations
|
|
|
|
from typing import TYPE_CHECKING
|
|
from uuid import UUID
|
|
|
|
from fastapi import APIRouter, Depends, HTTPException, Request, status
|
|
from pydantic import BaseModel, Field
|
|
|
|
from app.gateway.auth.repositories.base import LastActiveAdminError, LastAdminRemainsError, UserNotFoundError
|
|
from app.gateway.authz import assignable_role_names
|
|
from app.gateway.deps import get_user_repository, require_admin_user
|
|
|
|
if TYPE_CHECKING:
|
|
from app.gateway.auth.repositories.sqlite import SQLiteUserRepository
|
|
|
|
from app.gateway.auth.models import UserResponse
|
|
|
|
router = APIRouter(prefix="/api/v1/admin/users", tags=["admin-users"])
|
|
|
|
|
|
class UserAccountUpdate(BaseModel):
|
|
"""Partial account update: role and/or enable-disable.
|
|
|
|
Omitted fields are unchanged; each present field routes to its
|
|
field-scoped writer (update_system_role / set_disabled) so role,
|
|
credentials, and lifecycle state can never clobber one another.
|
|
"""
|
|
|
|
system_role: str | None = Field(None, min_length=1, max_length=16, description="Built-in (admin/user) or a role configured in the authorization provider's roles mapping")
|
|
disabled: bool | None = Field(None, description="Suspend (true) or restore (false) the account; every authentication surface rejects a disabled account")
|
|
|
|
|
|
def _to_response(user) -> UserResponse:
|
|
return UserResponse(
|
|
id=str(user.id),
|
|
email=str(user.email),
|
|
system_role=str(user.system_role),
|
|
needs_setup=bool(getattr(user, "needs_setup", False)),
|
|
oauth_provider=getattr(user, "oauth_provider", None),
|
|
disabled=bool(getattr(user, "disabled", False)),
|
|
)
|
|
|
|
|
|
@router.get("")
|
|
async def list_users(request: Request, repository: SQLiteUserRepository = Depends(get_user_repository)) -> list[UserResponse]:
|
|
"""List all users with their system role (admin-only)."""
|
|
await require_admin_user(request, detail="admin user listing requires an admin credential")
|
|
users = await repository.list_users()
|
|
return [_to_response(user) for user in users]
|
|
|
|
|
|
@router.patch("/{user_id}")
|
|
async def update_user_account(
|
|
request: Request,
|
|
user_id: UUID,
|
|
update: UserAccountUpdate,
|
|
repository: SQLiteUserRepository = Depends(get_user_repository),
|
|
) -> UserResponse:
|
|
"""Update a user's account: assign a role and/or enable/disable (admin-only).
|
|
|
|
The role must be a built-in ("admin"/"user") or declared in the
|
|
authorization provider's roles configuration — an unknown name would make
|
|
the user's principal fall through to ``default_role`` on every decision,
|
|
which is never what an operator intended. Demoting the last remaining
|
|
admin, and disabling the last remaining ACTIVE admin, are both rejected
|
|
so a deployment cannot lock itself out of management. Each present field
|
|
routes to its field-scoped, serialized writer; enforcement of the
|
|
disabled state happens at every authentication surface (password login,
|
|
JWT sessions, PATs, OAuth provisioning), not here.
|
|
"""
|
|
await require_admin_user(request, detail="account updates require an admin credential")
|
|
|
|
if update.system_role is None and update.disabled is None:
|
|
raise HTTPException(status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, detail="nothing to update: provide system_role and/or disabled")
|
|
|
|
if update.system_role is not None:
|
|
assignable = assignable_role_names()
|
|
if update.system_role not in assignable:
|
|
raise HTTPException(
|
|
status_code=status.HTTP_422_UNPROCESSABLE_ENTITY,
|
|
detail=f"unknown role '{update.system_role}'; assignable roles: {sorted(assignable)}",
|
|
)
|
|
|
|
user: object | None = None
|
|
try:
|
|
if update.system_role is not None:
|
|
# Single serialized column write: the last-admin count and the
|
|
# write share one transaction (two concurrent demotions cannot
|
|
# both pass), and credential writers can never race this — see
|
|
# SQLiteUserRepository.update_system_role / update_user.
|
|
user = await repository.update_system_role(str(user_id), update.system_role)
|
|
if update.disabled is not None:
|
|
# Same serialization for the active-admin guard; re-enabling is
|
|
# always allowed.
|
|
user = await repository.set_disabled(str(user_id), update.disabled)
|
|
except UserNotFoundError:
|
|
raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail="user not found") from None
|
|
except LastAdminRemainsError:
|
|
raise HTTPException(status_code=status.HTTP_409_CONFLICT, detail="cannot demote the last remaining admin") from None
|
|
except LastActiveAdminError:
|
|
raise HTTPException(status_code=status.HTTP_409_CONFLICT, detail="cannot disable the last remaining active admin") from None
|
|
|
|
assert user is not None
|
|
return _to_response(user)
|