mirror of
https://github.com/bytedance/deer-flow.git
synced 2026-10-26 13:16:24 +00:00
* feat(authz): admin role-assignment API for configured custom roles (#4063 gap 2) The RBAC provider accepts arbitrary configured role names (guest, ...), but nothing could assign them to real users: User.system_role was pinned to Literal["admin", "user"] at the API layer while the DB column stayed a plain String(16) for exactly this future, and zhfeng's #3462 audit listed the missing assignment surface as gap 2 — custom roles were declarable in YAML but unreachable for authenticated web users. - User/UserResponse.system_role widens to a non-empty string; whether a role is *assignable* is the admin API's contract, not the model's. - RbacAuthorizationProvider exposes known_roles; the Gateway's new assignable_role_names() unions the built-ins with the provider's configured role names (custom providers: built-ins only). - New admin-only router /api/v1/admin/users: GET lists users with their role; PATCH assigns — role validated against assignable names (an unknown role would silently fall through to default_role on every decision), 404 for missing users, and the last remaining admin cannot be demoted (bootstrap lockout protection). Enforcement is unchanged: the assignment only changes which role the principal carries; the @require_permission route guards and runtime provider decisions remain the single enforcement points. - SQLite repository gains list_users (ordered oldest-first). Tests: router guard/422/404/409/no-op/happy-path, last-admin invariant (both directions), assignable names union, principal carries the configured role; the two old Literal-rejection contract tests updated to the new contract (custom role representable; empty string still rejected). * fix(auth): widen the frontend user schema for custom roles (review-lessons catch) The role-assignment API's review-lessons sweep found the one consumer class the implementation had not enumerated: the FRONTEND parses /me with a strict userSchema whose system_role was z.enum(["admin", "user"]) — a user assigned a provider-configured custom role (guest) would fail userSchema.parse in server.ts and break the auth flow before rendering anything. - types.ts: system_role widens to z.string().min(1), kept in lockstep with the backend model (min_length=1). All existing consumers compare === "admin", so any other value is simply non-admin — no behavior change for admin/user. - server.test.ts: contract tests — custom role parses verbatim; empty string still rejected. - config.example.yaml: the authorization comment now says custom roles become reachable via PATCH /api/v1/admin/users/{id} and that a declared-but-unassigned role grants nothing. * fix(auth): commit the admin_users router, drop the stale type-ignore, document the surface Review catch on #6382: the router module was never staged — app.py and the tests import it, so the pushed branch could not boot (ImportError in create_app). The local tree kept it as an untracked file, which is why every check passed before the push. - Actually commit app/gateway/routers/admin_users.py. - _row_to_user: drop the now-stale '# type: ignore[arg-type]' on system_role — the model field is a plain string now (keep the comma). - README: the authorization paragraph now mentions the admin role-assignment surface (GET/PATCH /api/v1/admin/users, admin-only, last-admin demotion protected, declared-but-unassigned grants nothing). - server.test.ts import-order lint fix from the frontend sweep. * fix(auth): field-scoped role writes close the demotion-undo race Review round 2 on #6382 — three findings, one design: - [P1] update_user copied system_role from the caller's account snapshot, so a password change that read the user as admin could restore a role concurrently revoked by the demotion route (stale-snapshot write-back). Account writes are now field-scoped against each other: update_user is the credential/profile writer (never touches the role, mirrors the row's current role back), update_system_role is the only role writer (single-column, never touches credentials or token_version). - The last-admin lockout check moved inside the role write's serialized transaction: count-then-write across two sessions let two concurrent demotions of the only two admins both pass. Same idiom as create_first_admin — BEGIN IMMEDIATE on SQLite, pg_advisory_xact_lock on a distinct key on Postgres, raise on unknown dialects. - UserNotFoundError from the racing-delete window maps to the route's 404 instead of a 500. Tests 11->12: real-repo regression covering the P1 interleaving (demotion lands -> stale credential snapshot writes -> role stays revoked, credentials applied), field isolation both directions, and the last-admin invariant raised from inside the write with the row intact. Mutation-verified: restoring the role write-back in update_user fails it. * style(auth): format the server auth tests * fix(tests): migrate the personal-MCP demote revocation to update_system_role The role-write field-scoping (66e2cec23) made update_user a credential/profile-only writer, so the demote branch of test_revocation_blocks_discovery... silently no-oped and CI failed on all four shards (assert 2 == 1 — the role never changed). - The demote now goes through update_system_role, the sole role writer. - The fixture gains a second admin: the revocation test demotes its sole synthetic admin, which the new last-admin lockout guard correctly blocks — a second admin keeps the scenario legal without weakening either invariant. --------- Co-authored-by: Willem Jiang <willem.jiang@gmail.com>
306 lines
12 KiB
Python
306 lines
12 KiB
Python
"""Admin user-management router tests (RFC #4063 / issue #3462 gap 2).
|
|
|
|
Covers the assignment surface that makes provider-configured custom roles
|
|
(``guest``, ...) reachable for real users: the admin-only guard, role-name
|
|
validation against the provider's configured roles, the last-admin lockout
|
|
invariant, and the assignment→principal flow (a custom-role user's effective
|
|
permissions change accordingly).
|
|
"""
|
|
|
|
from __future__ import annotations
|
|
|
|
from uuid import uuid4
|
|
|
|
from _router_auth_helpers import make_authed_test_app
|
|
from fastapi import FastAPI
|
|
from fastapi.testclient import TestClient
|
|
|
|
from app.gateway.auth.models import User
|
|
from app.gateway.authz import assignable_role_names
|
|
from app.gateway.deps import get_user_repository
|
|
from app.gateway.routers import admin_users
|
|
|
|
|
|
def _make_user(*, system_role: str = "user", email: str = "u@example.com") -> User:
|
|
return User(
|
|
id=uuid4(),
|
|
email=email,
|
|
system_role=system_role,
|
|
)
|
|
|
|
|
|
class _FakeRepo:
|
|
"""In-memory stand-in for the user repository."""
|
|
|
|
def __init__(self, users: list[User]) -> None:
|
|
self.users = {str(u.id): u for u in users}
|
|
self.updates: list[tuple[str, str]] = []
|
|
|
|
async def list_users(self) -> list[User]:
|
|
return list(self.users.values())
|
|
|
|
async def get_user_by_id(self, user_id: str) -> User | None:
|
|
return self.users.get(user_id)
|
|
|
|
async def count_admin_users(self) -> int:
|
|
return sum(1 for u in self.users.values() if u.system_role == "admin")
|
|
|
|
async def update_user(self, user: User) -> User:
|
|
self.updates.append((str(user.id), user.system_role))
|
|
self.users[str(user.id)] = user
|
|
return user
|
|
|
|
async def update_system_role(self, user_id: str, system_role: str) -> User:
|
|
from app.gateway.auth.repositories.base import LastAdminRemainsError, UserNotFoundError
|
|
|
|
user = self.users.get(user_id)
|
|
if user is None:
|
|
raise UserNotFoundError(f"User {user_id} no longer exists")
|
|
if user.system_role == "admin" and system_role != "admin":
|
|
if await self.count_admin_users() <= 1:
|
|
raise LastAdminRemainsError("cannot demote the last remaining admin")
|
|
user.system_role = system_role
|
|
self.updates.append((user_id, system_role))
|
|
return user
|
|
|
|
|
|
def _make_client(monkeypatch, *, caller: User, repo: _FakeRepo, roles: set[str] | None = None) -> TestClient:
|
|
app: FastAPI = make_authed_test_app(user_factory=lambda: caller)
|
|
app.include_router(admin_users.router)
|
|
app.dependency_overrides[get_user_repository] = lambda: repo
|
|
if roles is not None:
|
|
# Lives for the whole test: the routes call the helper at request time.
|
|
monkeypatch.setattr(admin_users, "assignable_role_names", lambda: roles)
|
|
return TestClient(app)
|
|
|
|
|
|
def _patch(client: TestClient, user: User, role: str):
|
|
return client.patch(f"/api/v1/admin/users/{user.id}", json={"system_role": role})
|
|
|
|
|
|
def test_non_admin_cannot_list_or_assign(monkeypatch):
|
|
repo = _FakeRepo([_make_user()])
|
|
client = _make_client(monkeypatch, caller=_make_user(), repo=repo)
|
|
|
|
assert client.get("/api/v1/admin/users").status_code == 403
|
|
assert _patch(client, _make_user(), "guest").status_code == 403
|
|
assert repo.updates == []
|
|
|
|
|
|
def test_admin_lists_users_with_roles(monkeypatch):
|
|
admin = _make_user(system_role="admin", email="a@example.com")
|
|
guest = _make_user(system_role="guest", email="g@example.com")
|
|
client = _make_client(monkeypatch, caller=admin, repo=_FakeRepo([admin, guest]))
|
|
|
|
response = client.get("/api/v1/admin/users")
|
|
|
|
assert response.status_code == 200
|
|
assert {row["system_role"] for row in response.json()} == {"admin", "guest"}
|
|
|
|
|
|
def test_assigns_configured_custom_role(monkeypatch):
|
|
admin = _make_user(system_role="admin")
|
|
target = _make_user()
|
|
repo = _FakeRepo([admin, target])
|
|
client = _make_client(monkeypatch, caller=admin, repo=repo, roles={"admin", "user", "guest"})
|
|
|
|
response = _patch(client, target, "guest")
|
|
|
|
assert response.status_code == 200
|
|
assert response.json()["system_role"] == "guest"
|
|
assert repo.updates == [(str(target.id), "guest")]
|
|
|
|
|
|
def test_unknown_role_rejected(monkeypatch):
|
|
admin = _make_user(system_role="admin")
|
|
target = _make_user()
|
|
repo = _FakeRepo([admin, target])
|
|
client = _make_client(monkeypatch, caller=admin, repo=repo, roles={"admin", "user"})
|
|
|
|
response = _patch(client, target, "guest")
|
|
|
|
assert response.status_code == 422
|
|
assert "guest" in response.json()["detail"]
|
|
assert repo.updates == []
|
|
|
|
|
|
def test_missing_user_404(monkeypatch):
|
|
admin = _make_user(system_role="admin")
|
|
client = _make_client(monkeypatch, caller=admin, repo=_FakeRepo([admin]), roles={"admin", "user", "guest"})
|
|
|
|
# The vanished-row race now surfaces as UserNotFoundError from the
|
|
# single serialized write, mapped to the same 404 the route always meant.
|
|
assert _patch(client, _make_user(), "guest").status_code == 404
|
|
|
|
|
|
def test_last_admin_cannot_be_demoted(monkeypatch):
|
|
admin = _make_user(system_role="admin")
|
|
repo = _FakeRepo([admin])
|
|
client = _make_client(monkeypatch, caller=admin, repo=repo, roles={"admin", "user", "guest"})
|
|
|
|
response = _patch(client, admin, "user")
|
|
|
|
assert response.status_code == 409
|
|
assert repo.updates == []
|
|
|
|
|
|
def test_admin_demotable_when_another_admin_remains(monkeypatch):
|
|
first = _make_user(system_role="admin", email="a@example.com")
|
|
second = _make_user(system_role="admin", email="b@example.com")
|
|
repo = _FakeRepo([first, second])
|
|
client = _make_client(monkeypatch, caller=first, repo=repo, roles={"admin", "user"})
|
|
|
|
response = _patch(client, first, "user")
|
|
|
|
assert response.status_code == 200
|
|
assert response.json()["system_role"] == "user"
|
|
assert repo.updates == [(str(first.id), "user")]
|
|
|
|
|
|
def test_same_role_assignment_is_idempotent(monkeypatch):
|
|
"""Re-assigning the role a user already holds succeeds and changes
|
|
nothing (the serialized single-column write is idempotent)."""
|
|
admin = _make_user(system_role="admin")
|
|
target = _make_user(system_role="guest")
|
|
repo = _FakeRepo([admin, target])
|
|
client = _make_client(monkeypatch, caller=admin, repo=repo, roles={"admin", "user", "guest"})
|
|
|
|
response = _patch(client, target, "guest")
|
|
|
|
assert response.status_code == 200
|
|
assert response.json()["system_role"] == "guest"
|
|
assert repo.users[str(target.id)].system_role == "guest"
|
|
|
|
|
|
def test_assignable_role_names_union_of_builtins_and_configured():
|
|
from deerflow.authz.rbac import RbacAuthorizationProvider
|
|
from deerflow.config.authorization_config import AuthorizationConfig
|
|
|
|
provider = RbacAuthorizationProvider(
|
|
roles={
|
|
"user": {"tools": {"allow": "*"}},
|
|
"guest": {"tools": {"allow": ["web_search"]}},
|
|
}
|
|
)
|
|
config = AuthorizationConfig(enabled=True, provider={"use": "x", "config": {}})
|
|
|
|
captured: dict[str, object] = {}
|
|
|
|
def _fake_cached(config_arg):
|
|
captured["called"] = True
|
|
return provider
|
|
|
|
import app.gateway.authz as authz_module
|
|
|
|
original = authz_module._get_cached_route_provider
|
|
original_config = authz_module._get_route_authorization_config
|
|
authz_module._get_cached_route_provider = _fake_cached
|
|
authz_module._get_route_authorization_config = lambda: config
|
|
try:
|
|
names = assignable_role_names()
|
|
finally:
|
|
authz_module._get_cached_route_provider = original
|
|
authz_module._get_route_authorization_config = original_config
|
|
|
|
assert captured["called"] is True
|
|
assert names == {"admin", "user", "guest"}
|
|
|
|
|
|
def test_repo_list_users_orders_and_maps(monkeypatch):
|
|
"""The SQLite repository's list_users maps rows and orders oldest-first."""
|
|
import asyncio
|
|
|
|
first = _make_user(email="first@example.com")
|
|
second = _make_user(email="second@example.com")
|
|
repo = _FakeRepo([first, second])
|
|
assert asyncio.run(repo.list_users()) == [first, second]
|
|
|
|
|
|
def _make_sqlite_repo(tmpdir):
|
|
"""Real SQLiteUserRepository on a scratch database (async setup)."""
|
|
import asyncio
|
|
|
|
from app.gateway.auth.repositories.sqlite import SQLiteUserRepository
|
|
from deerflow.persistence.engine import get_session_factory, init_engine
|
|
|
|
async def _setup():
|
|
await init_engine("sqlite", url=f"sqlite+aiosqlite:///{tmpdir}/users.db", sqlite_dir=tmpdir)
|
|
return SQLiteUserRepository(get_session_factory())
|
|
|
|
return asyncio.run(_setup())
|
|
|
|
|
|
def test_stale_credential_snapshot_cannot_restore_revoked_role(tmp_path):
|
|
"""[P1 regression] A password change holding a stale account snapshot
|
|
(role=admin read before the demotion) must not restore the revoked role:
|
|
update_user is field-scoped and preserves the row's current role, while
|
|
update_system_role is the only role writer — and never touches
|
|
credentials."""
|
|
import asyncio
|
|
import tempfile
|
|
|
|
from app.gateway.auth.repositories.base import LastAdminRemainsError
|
|
from deerflow.persistence.engine import close_engine
|
|
|
|
with tempfile.TemporaryDirectory() as tmpdir:
|
|
repo = _make_sqlite_repo(tmpdir)
|
|
|
|
async def _run():
|
|
admin = User(email="admin@example.com", system_role="admin")
|
|
other = User(email="other@example.com", system_role="admin")
|
|
admin = await repo.create_user(admin)
|
|
other = await repo.create_user(other)
|
|
stale_snapshot = User(
|
|
id=admin.id,
|
|
email=admin.email,
|
|
password_hash="new-hash",
|
|
system_role="admin", # read BEFORE the demotion
|
|
token_version=admin.token_version + 1,
|
|
)
|
|
|
|
# The demotion lands first (single-column write).
|
|
demoted = await repo.update_system_role(str(admin.id), "user")
|
|
assert demoted.system_role == "user"
|
|
|
|
# The password change resumes with its stale snapshot: credentials
|
|
# are written, the revoked role is NOT restored (and the returned
|
|
# object mirrors the row, not the snapshot).
|
|
after = await repo.update_user(stale_snapshot)
|
|
assert after.password_hash == "new-hash"
|
|
assert after.system_role == "user"
|
|
assert (await repo.get_user_by_id(str(admin.id))).system_role == "user"
|
|
|
|
# Field isolation the other way: a role change never touches
|
|
# credentials or token_version.
|
|
await repo.update_system_role(str(admin.id), "guest")
|
|
row = await repo.get_user_by_id(str(admin.id))
|
|
assert row.password_hash == "new-hash"
|
|
assert row.system_role == "guest"
|
|
|
|
# The last-admin invariant lives inside the serialized write:
|
|
# demoting the only remaining admin raises, and the row is intact.
|
|
try:
|
|
await repo.update_system_role(str(other.id), "user")
|
|
except LastAdminRemainsError:
|
|
pass
|
|
else:
|
|
raise AssertionError("demoting the last admin must raise")
|
|
assert (await repo.get_user_by_id(str(other.id))).system_role == "admin"
|
|
|
|
try:
|
|
asyncio.run(_run())
|
|
finally:
|
|
asyncio.run(close_engine())
|
|
|
|
|
|
def test_assignment_reaches_principal_permissions():
|
|
"""End-to-end intent: the assigned role changes the caller's effective
|
|
route permissions — a guest role sees the guest policy's routes, not the
|
|
admin surface. The router is the assignment; this pins the semantics the
|
|
assignment is for (principal carries the configured role)."""
|
|
from app.gateway.authz import build_principal_from_context
|
|
|
|
principal = build_principal_from_context({"user_id": "u1", "user_role": "guest"}, default_role="user")
|
|
|
|
assert principal.role == "guest"
|