diff --git a/docs/content/Deploying/OIDC-SSO.mdx b/docs/content/Deploying/OIDC-SSO.mdx index 6282b104..0df8b73b 100644 --- a/docs/content/Deploying/OIDC-SSO.mdx +++ b/docs/content/Deploying/OIDC-SSO.mdx @@ -190,7 +190,9 @@ Authentication activity is recorded in `auth_events`, an append-only Postgres ta - `actor_id` — who performed the action. Never empty. - `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). -They differ whenever someone acts on someone else: an admin deactivating an account is `actor_id = `, `target_id = `. For self-service events such as a login they are the same user. Automated actors are named `system:oidc` (a group-driven role change at login) and `system:scim` (provisioning). `actor_id` defaults to `unknown`, which only a process from a release predating the column can produce. +They differ whenever someone acts on someone else: an admin deactivating an account is `actor_id = `, `target_id = `. For self-service events such as a login they are the same user. Automated actors are named `system:oidc` (a group-driven role change at login) and `system:scim` (provisioning). + +A `BEFORE INSERT` trigger fills both columns for any row that arrives without an `actor_id`, applying the same rules as the migration's backfill. That keeps attribution intact for a writer from a release that predates the columns — during a rolling deployment, for instance — instead of rejecting the insert or flattening the actor to a sentinel. | Event | Recorded when | | --- | --- | diff --git a/docsgpt/alembic/versions/0034_auth_events_actor_target.py b/docsgpt/alembic/versions/0034_auth_events_actor_target.py index 91887209..f3eb0877 100644 --- a/docsgpt/alembic/versions/0034_auth_events_actor_target.py +++ b/docsgpt/alembic/versions/0034_auth_events_actor_target.py @@ -24,6 +24,11 @@ Backfill rules, applied only to rows that predate the columns: the actor and have no single user target: ``team.*``, and instance- or team-scoped ``quota_policy_*`` changes. +The same two rules are installed as SQL functions and applied by a BEFORE +INSERT trigger to any row that arrives without an ``actor_id``, so a writer +from a release that predates these columns keeps its attribution instead of +being rejected or flattened to a sentinel. + 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 @@ -50,18 +55,32 @@ 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. + # The derivation rules, as functions, so the backfill below and the + # compatibility trigger share one definition instead of two copies of the + # same CASE drifting apart. op.execute( """ - UPDATE auth_events - SET actor_id = COALESCE( + CREATE OR REPLACE FUNCTION auth_events_derive_actor( + metadata jsonb, user_id text + ) RETURNS text + LANGUAGE sql IMMUTABLE AS $$ + SELECT COALESCE( metadata->>'by', metadata->>'granted_by', metadata->>'revoked_by', user_id - ), - target_id = CASE + ); + $$; + """ + ) + op.execute( + """ + CREATE OR REPLACE FUNCTION auth_events_derive_target( + event text, metadata jsonb, user_id text + ) RETURNS text + LANGUAGE sql IMMUTABLE AS $$ + SELECT CASE + -- Team events were always filed under the actor. WHEN event LIKE 'team.%' THEN NULL -- An instance or team quota policy is filed under the acting -- admin, not a target user (see api/admin/quotas.py::_audit); @@ -69,22 +88,71 @@ def upgrade() -> None: WHEN event IN ('quota_policy_set', 'quota_policy_deleted') AND COALESCE(metadata->>'scope', '') <> 'user' THEN NULL ELSE user_id - END + END; + $$; + """ + ) + + # 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 = auth_events_derive_actor(metadata, user_id), + target_id = auth_events_derive_target(event, metadata, user_id) 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``). + # A writer from a release that predates these columns names only the old + # ones, so ``actor_id`` arrives NULL. Two things follow, and the trigger + # handles both: # - # The DEFAULT is what makes this safe during a rolling deploy: a - # still-running previous-release process inserts (user_id, event, ip, - # user_agent, metadata) with no actor, and those inserts share the - # request's transaction in admin/routes.py -- a NotNullViolation there - # would 500 the deactivate or role grant and roll its write back with it. - # 'unknown' is the same sentinel ``record_event`` writes for an - # unauthenticated actor, so the column stays readable either way. - op.execute("ALTER TABLE auth_events ALTER COLUMN actor_id SET DEFAULT 'unknown';") + # 1. The NOT NULL below would reject the insert. In admin/routes.py that + # insert shares the request's transaction, so a NotNullViolation would + # 500 the deactivate or role grant and roll its write back with it. + # 2. Defaulting it to a sentinel would keep the write but lose the + # attribution -- and the highest-volume events are the recoverable + # ones: for a login or a silent renewal the actor is simply the user. + # + # ``actor_id IS NULL`` identifies a legacy insert exactly, because the + # repository always supplies one. That matters for ``target_id``: a current + # writer sets it to NULL deliberately for events with no user target + # (``conversation.deleted``), and deriving there would wrongly name the + # actor as their own target. + # + # Kept rather than dropped in a follow-up: it costs nothing on the path the + # repository takes, and it keeps the column's contract true for any writer + # that bypasses it. + op.execute( + """ + CREATE OR REPLACE FUNCTION auth_events_fill_attribution() + RETURNS trigger + LANGUAGE plpgsql AS $$ + BEGIN + IF NEW.actor_id IS NULL THEN + NEW.actor_id := auth_events_derive_actor( + NEW.metadata, NEW.user_id + ); + NEW.target_id := auth_events_derive_target( + NEW.event, NEW.metadata, NEW.user_id + ); + END IF; + RETURN NEW; + END; + $$; + """ + ) + op.execute("DROP TRIGGER IF EXISTS auth_events_fill_attribution_trg ON auth_events;") + op.execute( + """ + CREATE TRIGGER auth_events_fill_attribution_trg + BEFORE INSERT ON auth_events + FOR EACH ROW EXECUTE FUNCTION auth_events_fill_attribution(); + """ + ) + + # Backfilled above and guaranteed by the trigger, so this is safe to assert. op.execute("ALTER TABLE auth_events ALTER COLUMN actor_id SET NOT NULL;") op.execute( @@ -105,5 +173,9 @@ 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("DROP TRIGGER IF EXISTS auth_events_fill_attribution_trg ON auth_events;") + op.execute("DROP FUNCTION IF EXISTS auth_events_fill_attribution();") op.execute("ALTER TABLE auth_events DROP COLUMN IF EXISTS target_id;") op.execute("ALTER TABLE auth_events DROP COLUMN IF EXISTS actor_id;") + op.execute("DROP FUNCTION IF EXISTS auth_events_derive_target(text, jsonb, text);") + op.execute("DROP FUNCTION IF EXISTS auth_events_derive_actor(jsonb, text);") diff --git a/tests/storage/db/test_migration_0034.py b/tests/storage/db/test_migration_0034.py index 27aeb007..12d82e19 100644 --- a/tests/storage/db/test_migration_0034.py +++ b/tests/storage/db/test_migration_0034.py @@ -126,24 +126,61 @@ class TestMigration0034RoundTrip: # Team events were always filed under the actor; they have no user target. assert rows["team.create"] == "actor-3|-" - def test_actor_id_has_a_default_for_rolling_deploys(self, pg_engine): - """A previous-release process must not 500 mid-rollout. + def test_legacy_insert_keeps_its_attribution(self, pg_engine): + """A previous-release writer must neither 500 nor lose attribution. - Its inserts name only the old columns; without a DEFAULT the NOT NULL - raises, and in admin/routes.py that insert shares the request's - transaction, so the role grant beside it would roll back too. + Its inserts name only the old columns, so ``actor_id`` arrives NULL. + The NOT NULL alone would reject them -- and in admin/routes.py that + insert shares the request's transaction, so the role grant beside it + would roll back too. A bare sentinel would keep the row but throw the + actor away, and the highest-volume events are the recoverable ones. """ with pg_engine.begin() as conn: conn.execute( text( - "INSERT INTO auth_events (user_id, event, metadata) " - "VALUES ('legacy', 'oidc_login', '{}'::jsonb)" + "INSERT INTO auth_events (user_id, event, metadata) VALUES " + # A login: the user is their own actor and target. + "('legacy-login', 'oidc_login', '{}'::jsonb), " + # An admin action: the actor is recoverable from metadata. + "('legacy-victim', 'admin_user_deactivated', " + " '{\"by\": \"admin-legacy\"}'::jsonb), " + # A team event: filed under the actor, no user target. + "('legacy-actor', 'team.create', '{}'::jsonb)" ) ) - actor = conn.execute( - text("SELECT actor_id FROM auth_events WHERE user_id = 'legacy'") - ).scalar() - assert actor == "unknown" + rows = dict( + conn.execute( + text( + "SELECT user_id, actor_id || '|' || COALESCE(target_id, '-') " + "FROM auth_events WHERE user_id LIKE 'legacy-%'" + ) + ).fetchall() + ) + assert rows["legacy-login"] == "legacy-login|legacy-login" + assert rows["legacy-victim"] == "admin-legacy|legacy-victim" + assert rows["legacy-actor"] == "legacy-actor|-" + + def test_trigger_leaves_a_current_writer_alone(self, pg_engine): + """A deliberate NULL target must survive. + + ``conversation.deleted`` acts on a resource, not an account. Deriving + a target there would name the actor as their own target. + """ + with pg_engine.begin() as conn: + conn.execute( + text( + "INSERT INTO auth_events " + "(user_id, actor_id, target_id, event, metadata) " + "VALUES ('u9', 'u9', NULL, 'conversation.deleted', '{}'::jsonb)" + ) + ) + row = conn.execute( + text( + "SELECT actor_id, target_id FROM auth_events " + "WHERE event = 'conversation.deleted'" + ) + ).fetchone() + assert tuple(row) == ("u9", None) def test_backfill_scopes_quota_policy_targets(self, pg_engine): """Only a user-scoped quota policy has a user target.