feat(audit): split actor from subject on auth events

auth_events.user_id was overloaded and meant something different per call
site: admin mutations filed the row under the target user and hid the acting
admin in metadata->>'by', team events filed it under the actor, and quota
events switched between the two depending on scope. The practical consequence
was that "show me everything admin X did" had no answer.

Adds actor_id (never NULL) and target_id (NULL when the event is not about a
user) alongside the existing column, which keeps its meaning as the per-user
feed key so nothing historical is rewritten. The backfill recovers the actor
from metadata for rows that predate the columns.

Also adds the indexes the cross-user admin feed needs. The only index was
(user_id, created_at DESC), so the global feed -- which orders by created_at
with no user predicate -- fell back to a sequential scan plus a sort on every
page.

The feed gains actor, multi-event, until and free-text search filters, plus a
distinct-event catalogue so the UI can offer what the instance recorded
instead of asking operators to type a name from memory.
This commit is contained in:
arc53-machine committed 2026-09-22 10:18:16 +01:00
1 parent f5b02ed374
commit cb5343397f
11 files changed
+483 -15

No files matched your search

@@ -0,0 +1,94 @@
"""0034 auth events actor/target — who did it, and to whom.
``auth_events.user_id`` was overloaded: admin mutations file the row under the
*target* user and hide the acting admin in ``metadata->>'by'``, team events file
it under the *actor*, and quota events switch between the two depending on
scope. The practical consequence is that "show me everything admin X did" is
unanswerable — the first question any audit review asks.
This adds two explicit columns:
* ``actor_id`` — who performed the action (never NULL going forward).
* ``target_id`` — the user the action was performed on; NULL when the event is
not about a user (a team or an instance-wide policy change).
``user_id`` is deliberately left alone. It stays the per-user feed key
(``list_recent``) and every historical row keeps its meaning, so this migration
is additive and the backfill never rewrites it.
Backfill rules, applied only to rows that predate the columns:
* ``actor_id`` = the first present of ``metadata->>'by'``,
``metadata->>'granted_by'``, ``metadata->>'revoked_by'``, else ``user_id``.
* ``target_id`` = ``user_id``, except for ``team.*`` events, which were always
filed under the actor and have no single user target.
Also adds the indexes the global admin feed needs. Before this the only index
was ``(user_id, created_at DESC)``, so the cross-user feed — which orders by
``created_at DESC`` with no user predicate — degraded to a sequential scan plus
a sort on every page.
Idempotent both ways.
Revision ID: 0034_auth_events_actor_target
Revises: 0033_quotas
"""
from typing import Sequence, Union
from alembic import op
revision: str = "0034_auth_events_actor_target"
down_revision: Union[str, None] = "0033_quotas"
branch_labels: Union[str, Sequence[str], None] = None
depends_on: Union[str, Sequence[str], None] = None
def upgrade() -> None:
op.execute("ALTER TABLE auth_events ADD COLUMN IF NOT EXISTS actor_id TEXT;")
op.execute("ALTER TABLE auth_events ADD COLUMN IF NOT EXISTS target_id TEXT;")
# Backfill. ``actor_id IS NULL`` scopes this to pre-migration rows, so a
# re-run (or a downgrade/upgrade cycle) never clobbers written values.
op.execute(
"""
UPDATE auth_events
SET actor_id = COALESCE(
metadata->>'by',
metadata->>'granted_by',
metadata->>'revoked_by',
user_id
),
target_id = CASE
WHEN event LIKE 'team.%' THEN NULL
ELSE user_id
END
WHERE actor_id IS NULL;
"""
)
# Backfilled every row above, so the NOT NULL is safe to assert now. New
# rows always carry an actor (the repository derives one from ``user_id``).
op.execute("ALTER TABLE auth_events ALTER COLUMN actor_id SET NOT NULL;")
op.execute(
"CREATE INDEX IF NOT EXISTS auth_events_created_idx "
"ON auth_events (created_at DESC);"
)
op.execute(
"CREATE INDEX IF NOT EXISTS auth_events_event_created_idx "
"ON auth_events (event, created_at DESC);"
)
op.execute(
"CREATE INDEX IF NOT EXISTS auth_events_actor_idx "
"ON auth_events (actor_id, created_at DESC);"
)
def downgrade() -> None:
op.execute("DROP INDEX IF EXISTS auth_events_actor_idx;")
op.execute("DROP INDEX IF EXISTS auth_events_event_created_idx;")
op.execute("DROP INDEX IF EXISTS auth_events_created_idx;")
op.execute("ALTER TABLE auth_events DROP COLUMN IF EXISTS target_id;")
op.execute("ALTER TABLE auth_events DROP COLUMN IF EXISTS actor_id;")
+4
View File
@@ -119,6 +119,10 @@ def _audit(conn, event: str, scope: str, subject_id: Optional[str], detail: dict
ip=request.remote_addr,
user_agent=request.headers.get("User-Agent"),
metadata={"by": actor, "via": "admin_api", "scope": scope, "subject_id": subject_id, **detail},
actor_id=actor or "unknown",
# Only a user-scoped policy targets a user; instance and team policies
# change configuration, not an account.
target_id=subject_id if scope == "user" else None,
)
+10
View File
@@ -171,6 +171,8 @@ class AdminUserResource(Resource):
ip=request.remote_addr,
user_agent=request.headers.get("User-Agent"),
metadata={"by": actor, "via": "admin_api"},
actor_id=actor,
target_id=user_id,
)
if not active:
# Best-effort live-session revocation (mirrors SCIM deactivation).
@@ -203,6 +205,8 @@ class AdminUserRoleResource(Resource):
"granted_by": actor,
"via": "admin_api",
},
actor_id=actor,
target_id=user_id,
)
return make_response(
jsonify({"success": True, "granted": inserted, "role": ROLE_ADMIN}), 200
@@ -238,6 +242,8 @@ class AdminUserRoleResource(Resource):
"revoked_by": actor,
"via": "admin_api",
},
actor_id=actor,
target_id=user_id,
)
return make_response(jsonify({"success": True, "revoked": removed}), 200)
@@ -261,6 +267,8 @@ class AdminUserSessionsResource(Resource):
ip=request.remote_addr,
user_agent=request.headers.get("User-Agent"),
metadata={"token_id": token_id, "by": _actor(), "via": "admin_sessions_revoked"},
actor_id=_actor(),
target_id=user_id,
)
AuthEventsRepository(conn).insert(
user_id,
@@ -273,6 +281,8 @@ class AdminUserSessionsResource(Resource):
"persisted": ok,
"personal_access_tokens_revoked": len(revoked_token_ids),
},
actor_id=_actor(),
target_id=user_id,
)
return make_response(jsonify({"success": True, "revoked": ok}), 200)
+7 -1
View File
@@ -26,7 +26,10 @@ from docsgpt.api.oidc import denylist, provider
from docsgpt.auth import handle_auth
from docsgpt.cache import get_redis_instance
from docsgpt.core.settings import settings
from docsgpt.storage.db.repositories.auth_events import AuthEventsRepository
from docsgpt.storage.db.repositories.auth_events import (
SYSTEM_ACTOR_OIDC,
AuthEventsRepository,
)
from docsgpt.storage.db.repositories.user_roles import UserRolesRepository
from docsgpt.storage.db.repositories.users import UsersRepository
from docsgpt.storage.db.session import db_readonly, db_session
@@ -129,6 +132,9 @@ def _reconcile_oidc_admin(user_id: str, groups: list[str] | None) -> None:
ip=request.remote_addr,
user_agent=request.headers.get("User-Agent"),
metadata={"role": "admin", "source": "oidc_group"},
# The provider's group membership drove this, not the user.
actor_id=SYSTEM_ACTOR_OIDC,
target_id=user_id,
)
except Exception:
logger.error("OIDC admin reconcile failed for %s", user_id, exc_info=True)
+2
View File
@@ -307,6 +307,8 @@ class AdminToken(Resource):
ip=request.remote_addr,
user_agent=request.headers.get("User-Agent"),
metadata={"token_id": token_id, "by": actor, "via": "admin_api"},
actor_id=actor,
target_id=row["user_id"],
)
if not revoked:
return _error("Token not found", 404)
+11 -2
View File
@@ -20,7 +20,10 @@ from sqlalchemy import Connection
from docsgpt.api.oidc.denylist import deny_user
from docsgpt.core.settings import settings
from docsgpt.storage.db.repositories.auth_events import AuthEventsRepository
from docsgpt.storage.db.repositories.auth_events import (
SYSTEM_ACTOR_SCIM,
AuthEventsRepository,
)
from docsgpt.storage.db.repositories.users import UsersRepository
from docsgpt.storage.db.session import db_readonly, db_session
@@ -201,7 +204,13 @@ def _audit(conn: Connection, user_id: str, event: str) -> None:
"""Best-effort audit insert in a savepoint; failure never fails the request."""
try:
with conn.begin_nested():
AuthEventsRepository(conn).insert(user_id, event, metadata={"via": "scim"})
AuthEventsRepository(conn).insert(
user_id,
event,
metadata={"via": "scim"},
actor_id=SYSTEM_ACTOR_SCIM,
target_id=user_id,
)
except Exception:
logger.error("SCIM audit insert failed for user %s event %s", user_id, event, exc_info=True)
+16 -1
View File
@@ -62,13 +62,26 @@ def _current_user() -> str | None:
return token.get("sub") if isinstance(token, dict) else None
# Metadata keys naming the user a team event acted on, most specific first.
# Lets ``_audit`` fill ``target_id`` without every call site repeating it.
_TARGET_METADATA_KEYS = ("target_user", "target_user_id", "new_owner")
def _audit(conn, actor: str | None, event: str, **metadata) -> None:
"""Append a team management event to the audit trail (best-effort).
Runs inside the action's transaction so the audit row commits atomically
with the change. Never raises into the request path — an audit failure must
not fail the operation.
Team events are filed under the acting user. When the action names another
member (add, role change, removal, ownership transfer) that member becomes
the row's ``target_id``; otherwise the event has no user target.
"""
detail = {k: v for k, v in metadata.items() if v is not None}
target = next(
(detail[key] for key in _TARGET_METADATA_KEYS if detail.get(key)), None
)
try:
# SAVEPOINT: a failed audit insert poisons the surrounding txn, so
# nest it — on failure only the audit rolls back, not the action.
@@ -78,7 +91,9 @@ def _audit(conn, actor: str | None, event: str, **metadata) -> None:
event=event,
ip=request.remote_addr,
user_agent=request.headers.get("User-Agent"),
metadata={k: v for k, v in metadata.items() if v is not None},
metadata=detail,
actor_id=actor or "unknown",
target_id=target,
)
except Exception:
logger.warning("team audit insert failed for event=%s", event, exc_info=True)
+5
View File
@@ -68,6 +68,11 @@ auth_events_table = Table(
metadata,
Column("id", UUID(as_uuid=True), primary_key=True, server_default=func.gen_random_uuid()),
Column("user_id", Text, nullable=False),
# Who performed the action, and (when the action is about a user) whom it
# was performed on. ``user_id`` predates both and is kept as the per-user
# feed key; see migration 0034.
Column("actor_id", Text, nullable=False),
Column("target_id", Text),
Column("event", Text, nullable=False),
Column("ip", Text),
Column("user_agent", Text),
+122 -11
View File
@@ -3,15 +3,33 @@
from __future__ import annotations
import json
from typing import Optional
from typing import Any, Optional, Sequence
from sqlalchemy import Connection, text
from docsgpt.storage.db.base_repository import row_to_dict
# Actors that are not a person. Used where an automated system performs the
# action on a user's behalf, so the feed never reads as if the user did it.
SYSTEM_ACTOR_SCIM = "system:scim"
SYSTEM_ACTOR_OIDC = "system:oidc"
class AuthEventsRepository:
"""Append-only audit trail of login / logout / provisioning events."""
"""Append-only audit trail of identity, access and data-plane events.
Every row carries three attribution columns:
* ``actor_id`` — who performed the action. Never NULL.
* ``target_id`` — the user the action was performed on, or NULL when the
event is not about a user (a team change, an instance-wide policy).
* ``user_id`` — the legacy subject column, kept as the key for the
per-user feed (:meth:`list_recent`). Defaults to the target, falling
back to the actor for actor-only events.
See migration ``0034_auth_events_actor_target``.
"""
def __init__(self, conn: Connection) -> None:
self._conn = conn
@@ -23,18 +41,45 @@ class AuthEventsRepository:
ip: Optional[str] = None,
user_agent: Optional[str] = None,
metadata: Optional[dict] = None,
*,
actor_id: Optional[str] = None,
target_id: Optional[str] = "",
) -> dict:
"""Record one auth event and return the inserted row."""
"""Record one audit event and return the inserted row.
Args:
user_id: The row's subject, kept for the per-user feed.
event: Event name (``oidc_login``, ``source.deleted``, ...).
ip: Client IP, when the event came from a request.
user_agent: Client ``User-Agent``, when the event came from a request.
metadata: Free-form JSON detail shown in the audit drill-down.
actor_id: Who performed the action. Defaults to ``user_id``, which
is correct for self-service events (login, PAT create).
target_id: The user acted upon. Defaults to ``user_id``; pass
``None`` explicitly for events with no user target.
Returns:
The inserted row as a dict.
"""
# Sentinel default: ``None`` is a meaningful value here ("no target"),
# so it cannot double as "caller did not say".
resolved_target = user_id if target_id == "" else target_id
resolved_actor = actor_id or user_id
result = self._conn.execute(
text(
"""
INSERT INTO auth_events (user_id, event, ip, user_agent, metadata)
VALUES (:user_id, :event, :ip, :user_agent, CAST(:metadata AS jsonb))
INSERT INTO auth_events
(user_id, actor_id, target_id, event, ip, user_agent, metadata)
VALUES
(:user_id, :actor_id, :target_id, :event, :ip, :user_agent,
CAST(:metadata AS jsonb))
RETURNING *
"""
),
{
"user_id": user_id,
"actor_id": resolved_actor,
"target_id": resolved_target,
"event": event,
"ip": ip,
"user_agent": user_agent,
@@ -59,18 +104,45 @@ class AuthEventsRepository:
return [row_to_dict(row) for row in result.fetchall()]
@staticmethod
def _filter_clauses(event, user_id, since) -> tuple[str, dict]:
def _filter_clauses(
event: Optional[str],
user_id: Optional[str],
since: Any,
*,
events: Optional[Sequence[str]] = None,
actor_id: Optional[str] = None,
until: Any = None,
search: Optional[str] = None,
) -> tuple[str, dict]:
clauses: list[str] = []
params: dict = {}
if event:
clauses.append("event = :event")
params["event"] = event
if events:
clauses.append("event = ANY(:events)")
params["events"] = list(events)
if user_id:
clauses.append("user_id = :user_id")
clauses.append("(user_id = :user_id OR target_id = :user_id)")
params["user_id"] = user_id
if actor_id:
clauses.append("actor_id = :actor_id")
params["actor_id"] = actor_id
if since is not None:
clauses.append("created_at >= :since")
params["since"] = since
if until is not None:
clauses.append("created_at <= :until")
params["until"] = until
if search:
# Substring match across the human-meaningful columns plus the
# serialized metadata, so "ci-deploy-key" finds the PAT it named.
clauses.append(
"(user_id ILIKE :search OR actor_id ILIKE :search "
"OR target_id ILIKE :search OR event ILIKE :search "
"OR ip ILIKE :search OR metadata::text ILIKE :search)"
)
params["search"] = f"%{search}%"
where = ("WHERE " + " AND ".join(clauses)) if clauses else ""
return where, params
@@ -78,13 +150,25 @@ class AuthEventsRepository:
self,
*,
event: Optional[str] = None,
events: Optional[Sequence[str]] = None,
user_id: Optional[str] = None,
actor_id: Optional[str] = None,
since=None,
until=None,
search: Optional[str] = None,
limit: int = 50,
offset: int = 0,
) -> list[dict]:
"""Global audit feed (admin), newest first; optional event/user/since filters."""
where, params = self._filter_clauses(event, user_id, since)
"""Global audit feed (admin), newest first; see :meth:`_filter_clauses`."""
where, params = self._filter_clauses(
event,
user_id,
since,
events=events,
actor_id=actor_id,
until=until,
search=search,
)
params.update({"limit": int(limit), "offset": int(offset)})
result = self._conn.execute(
text(
@@ -96,13 +180,40 @@ class AuthEventsRepository:
return [row_to_dict(row) for row in result.fetchall()]
def count_all(
self, *, event: Optional[str] = None, user_id: Optional[str] = None, since=None
self,
*,
event: Optional[str] = None,
events: Optional[Sequence[str]] = None,
user_id: Optional[str] = None,
actor_id: Optional[str] = None,
since=None,
until=None,
search: Optional[str] = None,
) -> int:
"""Total matching the same filters as :meth:`list_all` (for pagination)."""
where, params = self._filter_clauses(event, user_id, since)
where, params = self._filter_clauses(
event,
user_id,
since,
events=events,
actor_id=actor_id,
until=until,
search=search,
)
return int(
self._conn.execute(
text(f"SELECT count(*) FROM auth_events {where}"), params
).scalar()
or 0
)
def event_names(self) -> list[str]:
"""Distinct event names present in the table, alphabetically.
Feeds the admin filter's event picker, so operators pick from what the
instance actually recorded instead of typing a name from memory.
"""
result = self._conn.execute(
text("SELECT DISTINCT event FROM auth_events ORDER BY event")
)
return [row[0] for row in result.fetchall()]
@@ -0,0 +1,85 @@
"""Repository tests for ``auth_events`` (actor/target attribution + feed filters)."""
from __future__ import annotations
from datetime import datetime, timedelta, timezone
import pytest
from docsgpt.storage.db.repositories.auth_events import AuthEventsRepository
pytestmark = pytest.mark.integration
@pytest.fixture()
def repo(pg_conn):
return AuthEventsRepository(pg_conn)
class TestInsertAttribution:
def test_self_service_event_is_its_own_actor(self, repo):
row = repo.insert("u1", "oidc_login")
assert row["actor_id"] == "u1"
assert row["target_id"] == "u1"
assert row["user_id"] == "u1"
def test_admin_action_separates_actor_from_target(self, repo):
row = repo.insert(
"victim", "admin_user_deactivated", actor_id="admin-1", target_id="victim"
)
assert row["actor_id"] == "admin-1"
assert row["target_id"] == "victim"
# ``user_id`` stays the subject, so the per-user feed is unchanged.
assert row["user_id"] == "victim"
def test_actor_only_event_has_no_target(self, repo):
row = repo.insert("admin-1", "team.create", actor_id="admin-1", target_id=None)
assert row["actor_id"] == "admin-1"
assert row["target_id"] is None
def test_positional_user_id_still_supported(self, repo):
"""Existing call sites pass only ``user_id``; it must keep working."""
row = repo.insert("u2", "pat_created", metadata={"token_id": "t"})
assert (row["actor_id"], row["target_id"]) == ("u2", "u2")
class TestFeedFilters:
def _seed(self, repo):
repo.insert("a", "oidc_login")
repo.insert("b", "oidc_login_denied")
repo.insert("b", "admin_user_deactivated", actor_id="admin-9", target_id="b")
def test_filter_by_actor(self, repo):
self._seed(repo)
rows = repo.list_all(actor_id="admin-9")
assert [r["event"] for r in rows] == ["admin_user_deactivated"]
assert repo.count_all(actor_id="admin-9") == 1
def test_filter_by_multiple_events(self, repo):
self._seed(repo)
rows = repo.list_all(events=["oidc_login", "oidc_login_denied"])
assert {r["event"] for r in rows} == {"oidc_login", "oidc_login_denied"}
assert repo.count_all(events=["oidc_login", "oidc_login_denied"]) == 2
def test_single_event_filter_still_supported(self, repo):
self._seed(repo)
assert repo.count_all(event="oidc_login") == 1
def test_filter_by_until(self, repo):
self._seed(repo)
past = datetime.now(timezone.utc) - timedelta(days=1)
assert repo.count_all(until=past) == 0
assert repo.count_all(until=datetime.now(timezone.utc) + timedelta(days=1)) == 3
def test_search_matches_user_actor_and_metadata(self, repo):
repo.insert("someone", "pat_created", metadata={"name": "ci-deploy-key"})
assert repo.count_all(search="ci-deploy") == 1
assert repo.count_all(search="someone") == 1
assert repo.count_all(search="nothing-here") == 0
def test_event_names_catalogue(self, repo):
self._seed(repo)
names = repo.event_names()
assert names == sorted(names)
assert "oidc_login" in names and "admin_user_deactivated" in names
+127
View File
@@ -0,0 +1,127 @@
"""Migration round-trip test for 0034_auth_events_actor_target."""
from __future__ import annotations
import os
import subprocess
import sys
from pathlib import Path
import pytest
from sqlalchemy import text
pytestmark = pytest.mark.integration
def _alembic_ini() -> Path:
return Path(__file__).resolve().parents[3] / "docsgpt" / "alembic.ini"
def _run_alembic(url: str, *args: str) -> None:
subprocess.check_call(
[sys.executable, "-m", "alembic", "-c", str(_alembic_ini()), *args],
timeout=60,
env={**os.environ, "POSTGRES_URI": url},
)
def _alembic_heads(url: str) -> list[str]:
out = subprocess.check_output(
[sys.executable, "-m", "alembic", "-c", str(_alembic_ini()), "heads"],
timeout=60,
env={**os.environ, "POSTGRES_URI": url},
text=True,
)
return [line for line in out.splitlines() if line.strip()]
def _alembic_version(conn) -> str:
return conn.execute(text("SELECT version_num FROM alembic_version")).scalar()
def _column_exists(conn, table: str, column: str) -> bool:
row = conn.execute(
text(
"SELECT 1 FROM information_schema.columns "
"WHERE table_name = :t AND column_name = :c AND table_schema = 'public'"
),
{"t": table, "c": column},
).fetchone()
return row is not None
def _index_exists(conn, name: str) -> bool:
row = conn.execute(
text("SELECT 1 FROM pg_indexes WHERE schemaname = 'public' AND indexname = :n"),
{"n": name},
).fetchone()
return row is not None
_0034 = "0034_auth_events_actor_target"
_0033 = "0033_quotas"
class TestMigration0034RoundTrip:
def test_single_head(self, pg_engine):
url = pg_engine.url.render_as_string(hide_password=False)
assert len(_alembic_heads(url)) == 1
def test_head_has_actor_target_columns(self, pg_engine):
with pg_engine.connect() as conn:
assert _alembic_version(conn) >= _0034
assert _column_exists(conn, "auth_events", "actor_id")
assert _column_exists(conn, "auth_events", "target_id")
def test_head_has_feed_indexes(self, pg_engine):
"""The global feed orders by created_at with no user filter."""
with pg_engine.connect() as conn:
assert _index_exists(conn, "auth_events_created_idx")
assert _index_exists(conn, "auth_events_event_created_idx")
assert _index_exists(conn, "auth_events_actor_idx")
def test_downgrade_drops_then_upgrade_restores(self, pg_engine):
url = pg_engine.url.render_as_string(hide_password=False)
_run_alembic(url, "downgrade", _0033)
with pg_engine.connect() as conn:
assert _alembic_version(conn) == _0033
assert not _column_exists(conn, "auth_events", "actor_id")
assert not _column_exists(conn, "auth_events", "target_id")
assert not _index_exists(conn, "auth_events_created_idx")
_run_alembic(url, "upgrade", "head")
with pg_engine.connect() as conn:
assert _alembic_version(conn) >= _0034
assert _column_exists(conn, "auth_events", "actor_id")
def test_backfill_reads_actor_out_of_metadata(self, pg_engine):
"""An admin action filed under its target keeps the acting admin."""
url = pg_engine.url.render_as_string(hide_password=False)
_run_alembic(url, "downgrade", _0033)
with pg_engine.begin() as conn:
conn.execute(
text(
"INSERT INTO auth_events (user_id, event, metadata) VALUES "
"('victim', 'admin_user_deactivated', '{\"by\": \"admin-1\"}'::jsonb), "
"('grantee', 'role_granted', '{\"granted_by\": \"admin-2\"}'::jsonb), "
"('someone', 'oidc_login', '{}'::jsonb), "
"('actor-3', 'team.create', '{\"team_id\": \"t1\"}'::jsonb)"
)
)
_run_alembic(url, "upgrade", "head")
with pg_engine.connect() as conn:
rows = dict(
conn.execute(
text(
"SELECT event, actor_id || '|' || COALESCE(target_id, '-') "
"FROM auth_events WHERE user_id IN "
"('victim', 'grantee', 'someone', 'actor-3')"
)
).fetchall()
)
assert rows["admin_user_deactivated"] == "admin-1|victim"
assert rows["role_granted"] == "admin-2|grantee"
# Self-service events: the user is both actor and target.
assert rows["oidc_login"] == "someone|someone"
# Team events were always filed under the actor; they have no user target.
assert rows["team.create"] == "actor-3|-"