Skip to content

Commit 4e145ef

Browse files
dwinter3claude
andcommitted
fix(personas): H-2/H-3/M-2/M-5 — audit table + admin guards + rate limit
Per #229 review: H-2: append-only persona_assignment_audit table. New migration 0023 chains from 0022. CREATED / CHANGED / DISABLED rows written in the same transaction as the upsert/disable so the audit and state never desync. HEAD_REVISION bumped to 0023_persona_assignment_audit; test_head_revision_is_current also updated (was stale at 0021 before this change). H-3: last-admin guard. POST .../disable refuses with HTTP 409 + code=LAST_ADMIN when the target is persona='admin' and active admin count <= 1. New store method count_active_admins() does the count. M-2: PATCH no longer silently re-enables a disabled assignment. Returns 409 + code=USER_DISABLED pointing the admin at the yet-to-land POST .../enable endpoint. M-5: per-admin invite rate limit. POST /admin/personas rejects with HTTP 429 + code=RATE_LIMIT when the calling admin has issued >= 20 assignments in the trailing hour. New store method count_invites_by_admin(). Tests added: * test_persona_changes_create_audit_rows * test_disabling_last_admin_returns_409_LAST_ADMIN * test_patch_disabled_user_returns_409_USER_DISABLED * test_admin_cannot_send_more_than_20_invites_per_hour Review: #229 (comment) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
1 parent bc9a355 commit 4e145ef

5 files changed

Lines changed: 296 additions & 17 deletions

File tree

Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,77 @@
1+
"""AS-1 follow-up: persona_assignment_audit — append-only history table.
2+
3+
Revision ID: 0023_persona_assignment_audit
4+
Revises: 0022_persona_assignments
5+
Create Date: 2026-05-12
6+
7+
The base 0022 table overwrites a single row per Human, which destroys the
8+
``assigned_by`` history every time an admin changes a persona. The audit
9+
table records every CREATED/CHANGED/DISABLED/ENABLED transition so the
10+
operator surface can answer "who set Carol to admin and when?"
11+
12+
# Chain note
13+
14+
After this migration lands, HEAD_REVISION in migrations.py must be
15+
``0023_persona_assignment_audit``.
16+
"""
17+
18+
from __future__ import annotations
19+
20+
from collections.abc import Sequence
21+
22+
import sqlalchemy as sa
23+
from alembic import op
24+
25+
revision: str = "0023_persona_assignment_audit"
26+
down_revision: str | Sequence[str] | None = "0022_persona_assignments"
27+
branch_labels: str | Sequence[str] | None = None
28+
depends_on: str | Sequence[str] | None = None
29+
30+
31+
def _table_exists(bind: sa.engine.Connection, table_name: str) -> bool:
32+
inspector = sa.inspect(bind)
33+
return table_name in inspector.get_table_names()
34+
35+
36+
def upgrade() -> None:
37+
"""Create the ``persona_assignment_audit`` table."""
38+
bind = op.get_bind()
39+
40+
if _table_exists(bind, "persona_assignment_audit"):
41+
return
42+
43+
op.create_table(
44+
"persona_assignment_audit",
45+
sa.Column("id", sa.Integer(), primary_key=True, autoincrement=True),
46+
sa.Column("username", sa.Text(), nullable=False),
47+
# old_persona is NULL on the CREATED row (no prior state).
48+
sa.Column("old_persona", sa.Text(), nullable=True),
49+
# new_persona is NULL on the DISABLED row (no live persona after).
50+
sa.Column("new_persona", sa.Text(), nullable=True),
51+
sa.Column("changed_by", sa.Text(), nullable=False),
52+
sa.Column(
53+
"changed_at",
54+
sa.DateTime(),
55+
nullable=False,
56+
server_default=sa.func.current_timestamp(),
57+
),
58+
sa.Column("action", sa.Text(), nullable=False),
59+
sa.CheckConstraint(
60+
"action IN ('CREATED', 'CHANGED', 'DISABLED', 'ENABLED')",
61+
name="ck_audit_action",
62+
),
63+
)
64+
op.create_index(
65+
"idx_audit_username",
66+
"persona_assignment_audit",
67+
["username", sa.text("changed_at DESC")],
68+
)
69+
70+
71+
def downgrade() -> None:
72+
"""Drop the audit table."""
73+
bind = op.get_bind()
74+
if not _table_exists(bind, "persona_assignment_audit"):
75+
return
76+
op.drop_index("idx_audit_username", table_name="persona_assignment_audit")
77+
op.drop_table("persona_assignment_audit")

server/backend/src/cq_server/migrations.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@
3636
# Phase 2 (task #100) — chain head after porting fork-delta tables to
3737
# Alembic. Update this string when adding a new migration so test
3838
# assertions and ops scripts stay in sync with the actual chain head.
39-
HEAD_REVISION = "0022_persona_assignments"
39+
HEAD_REVISION = "0023_persona_assignment_audit"
4040

4141

4242
def _find_alembic_ini() -> Path:

server/backend/src/cq_server/persona_routes.py

Lines changed: 59 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@
1919

2020
import logging
2121
import uuid
22-
from datetime import UTC, datetime
22+
from datetime import UTC, datetime, timedelta
2323
from typing import Literal
2424

2525
from fastapi import APIRouter, Depends, HTTPException, Query
@@ -180,6 +180,21 @@ async def create_persona(
180180
detail=f"persona assignment already exists for username={req.username!r}",
181181
)
182182

183+
# M-5: per-admin invite rate-limit. Count persona assignments this
184+
# admin has issued in the trailing hour; cap at 20.
185+
since = (datetime.now(UTC) - timedelta(hours=1)).isoformat()
186+
recent_count = await store.count_invites_by_admin(
187+
admin_username=admin, since=since
188+
)
189+
if recent_count >= 20:
190+
raise HTTPException(
191+
status_code=429,
192+
detail={
193+
"error": "Invite rate limit exceeded (20/hour).",
194+
"code": "RATE_LIMIT",
195+
},
196+
)
197+
183198
# Ensure the user row exists (or create it). We create a stub user
184199
# without a password hash — the invite claim flow will set the password.
185200
from .auth import hash_password
@@ -198,6 +213,8 @@ async def create_persona(
198213
persona=req.persona,
199214
assigned_at=now,
200215
assigned_by=admin,
216+
audit_action="CREATED",
217+
audit_old_persona=None,
201218
)
202219

203220
# Fire invite email (best-effort — same pattern as invite_routes).
@@ -255,12 +272,29 @@ async def patch_persona(
255272
if existing is None:
256273
raise HTTPException(status_code=404, detail=f"no persona assignment for {username!r}")
257274

275+
# M-2: don't silently re-enable a disabled user. PATCH is not the
276+
# enable path; admins should call POST /admin/personas/{username}/enable
277+
# (follow-up issue) once that lands.
278+
if existing.get("disabled_at") is not None:
279+
raise HTTPException(
280+
status_code=409,
281+
detail={
282+
"error": (
283+
"user is disabled — re-enable first via "
284+
"POST /admin/personas/{username}/enable"
285+
),
286+
"code": "USER_DISABLED",
287+
},
288+
)
289+
258290
now = datetime.now(UTC).isoformat()
259291
await store.upsert_persona_assignment(
260292
username=username,
261293
persona=req.persona,
262294
assigned_at=now,
263295
assigned_by=admin,
296+
audit_action="CHANGED",
297+
audit_old_persona=existing.get("persona"),
264298
)
265299
return PatchPersonaResponse(
266300
username=username,
@@ -273,23 +307,43 @@ async def patch_persona(
273307
@router.post("/{username}/disable", response_model=DisableResponse)
274308
async def disable_persona(
275309
username: str,
276-
admin: str = Depends(require_admin), # noqa: ARG001 — auth gate only
310+
admin: str = Depends(require_admin),
277311
store: SqliteStore = Depends(get_store),
278312
) -> DisableResponse:
279313
"""Soft-disable a Human's persona assignment.
280314
281-
Sets ``disabled_at`` to now. The users row is NOT deleted — audit trail
282-
is preserved. Re-enabling is done via PATCH (which clears disabled_at).
315+
Sets ``disabled_at`` to now. The users row is NOT deleted — audit
316+
trail is preserved. PATCH no longer silently re-enables (see M-2);
317+
a future POST .../enable endpoint owns the re-enable path.
283318
284319
404 when no assignment row exists.
285320
409 when the assignment is already disabled.
321+
409 + code=LAST_ADMIN when disabling would leave zero active admins.
286322
"""
287323
existing = await store.get_persona_assignment(username)
288324
if existing is None:
289325
raise HTTPException(status_code=404, detail=f"no persona assignment for {username!r}")
290326
if existing.get("disabled_at") is not None:
291327
raise HTTPException(status_code=409, detail=f"{username!r} is already disabled")
292328

329+
# H-3: last-admin guard. Refuse to disable the only remaining admin
330+
# so the L2 surface never loses its escape hatch.
331+
if existing.get("persona") == "admin":
332+
active_admins = await store.count_active_admins()
333+
if active_admins <= 1:
334+
raise HTTPException(
335+
status_code=409,
336+
detail={
337+
"error": "Cannot disable the last admin.",
338+
"code": "LAST_ADMIN",
339+
},
340+
)
341+
293342
now = datetime.now(UTC).isoformat()
294-
await store.disable_persona_assignment(username=username, disabled_at=now)
343+
await store.disable_persona_assignment(
344+
username=username,
345+
disabled_at=now,
346+
changed_by=admin,
347+
old_persona=existing.get("persona"),
348+
)
295349
return DisableResponse(username=username, disabled_at=now)

0 commit comments

Comments
 (0)