hataa 47d8ff13b2
feat(authz): account disable/enable across every authentication surface (#4063 gap 3) (#6566)
* 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>
2026-10-10 16:43:04 +08:00

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)