From 072405eb2dc66ca3f14b700ddec310b7b62838a8 Mon Sep 17 00:00:00 2001 From: Keith Smith Date: Thu, 27 Aug 2026 21:24:23 -0600 Subject: [PATCH] Make archiving a room actually affect existing members (#57) is_archived was previously only exposed on the admin-only AdminRoom schema and checked in one place (excluding a room from Browse rooms) -- for anyone already a member it was a complete no-op: still in their sidebar, still fully postable, no indication anywhere it was archived. Expose is_archived on the regular RoomRead/MyRoomItem schemas, drop archived rooms from the sidebar list (while keeping them directly reachable via URL so history stays readable), and reject new messages in one -- both the WS "message" handler and incoming webhooks -- with a clear "archived and read-only" response instead of silently no-op'ing or a confusing membership error. Co-Authored-By: Claude Sonnet 5 --- backend/app/routers/rooms.py | 2 + backend/app/routers/webhooks.py | 4 +- backend/app/schemas/room.py | 5 + backend/app/services/webhook_service.py | 9 ++ backend/app/ws/chat.py | 16 +++- backend/tests/test_archive.py | 121 ++++++++++++++++++++++++ frontend/src/components/ChatPane.tsx | 1 + frontend/src/components/Composer.tsx | 45 ++++++--- frontend/src/components/Sidebar.tsx | 6 +- frontend/src/types.ts | 5 + 10 files changed, 197 insertions(+), 17 deletions(-) create mode 100644 backend/tests/test_archive.py diff --git a/backend/app/routers/rooms.py b/backend/app/routers/rooms.py index e8297d6..3c039d8 100644 --- a/backend/app/routers/rooms.py +++ b/backend/app/routers/rooms.py @@ -147,6 +147,7 @@ async def list_rooms_endpoint( description=room.description, is_private=room.is_private, is_dm=room.is_dm, + is_archived=room.is_archived, owner_id=room.owner_id, created_at=room.created_at, is_member=is_member, @@ -171,6 +172,7 @@ async def list_my_rooms_endpoint( description=room.description, is_private=room.is_private, is_dm=room.is_dm, + is_archived=room.is_archived, owner_id=room.owner_id, created_at=room.created_at, role=role, diff --git a/backend/app/routers/webhooks.py b/backend/app/routers/webhooks.py index cc19e6d..c3f7e54 100644 --- a/backend/app/routers/webhooks.py +++ b/backend/app/routers/webhooks.py @@ -4,7 +4,7 @@ from sqlalchemy.ext.asyncio import AsyncSession from app.database import get_db from app.schemas.webhook import IncomingWebhookPost from app.services.message_events import broadcast_new_message -from app.services.webhook_service import WebhookNotFoundError, post_via_webhook +from app.services.webhook_service import RoomArchivedError, WebhookNotFoundError, post_via_webhook router = APIRouter(prefix="/api/webhooks", tags=["webhooks"]) @@ -22,6 +22,8 @@ async def incoming_webhook_endpoint( message, room, sender = await post_via_webhook(db, token, data.content) except WebhookNotFoundError: raise HTTPException(status_code=404, detail="Unknown webhook") + except RoomArchivedError: + raise HTTPException(status_code=403, detail="This room has been archived and is read-only") broadcaster = request.app.state.broadcaster presence = request.app.state.presence diff --git a/backend/app/schemas/room.py b/backend/app/schemas/room.py index 6a3326f..b239f75 100644 --- a/backend/app/schemas/room.py +++ b/backend/app/schemas/room.py @@ -27,6 +27,11 @@ class RoomRead(BaseModel): description: str | None is_private: bool is_dm: bool + # #57: previously only exposed on the admin-only AdminRoom schema, so a + # member of an archived room had no way to even know it was archived -- + # the flag was set server-side but had no effect on their own view of + # the room at all. + is_archived: bool owner_id: uuid.UUID created_at: datetime diff --git a/backend/app/services/webhook_service.py b/backend/app/services/webhook_service.py index 87c42bd..c8ce14f 100644 --- a/backend/app/services/webhook_service.py +++ b/backend/app/services/webhook_service.py @@ -27,6 +27,10 @@ class SubscriptionNotFoundError(Exception): pass +class RoomArchivedError(Exception): + pass + + async def create_incoming_webhook( db: AsyncSession, actor: User, room_id: uuid.UUID, description: str | None ) -> WebhookIncoming: @@ -82,6 +86,11 @@ async def post_via_webhook(db: AsyncSession, token: str, content: str) -> tuple[ webhook = result.scalar_one_or_none() if webhook is None: raise WebhookNotFoundError() + # #57: same read-only rule as a human posting from the composer -- an + # archived room shouldn't gain new messages through a bot integration + # either. + if webhook.room.is_archived: + raise RoomArchivedError() message = await create_message(db, webhook.room_id, webhook.created_by, content) return message, webhook.room, webhook.creator diff --git a/backend/app/ws/chat.py b/backend/app/ws/chat.py index a009222..de403fa 100644 --- a/backend/app/ws/chat.py +++ b/backend/app/ws/chat.py @@ -6,7 +6,7 @@ from sqlalchemy import select from sqlalchemy.ext.asyncio import AsyncSession from app.database import get_db -from app.models import ApiToken, Message, MessageFile, MessageImage, RoomMembership, User +from app.models import ApiToken, Message, MessageFile, MessageImage, Room, RoomMembership, User from app.services.bot_service import resolve_token from app.services.message_events import ( broadcast_dm_presence_update, @@ -49,6 +49,11 @@ async def _is_room_member(db: AsyncSession, room_id: uuid.UUID, user_id: uuid.UU return result.scalar_one_or_none() is not None +async def _is_room_archived(db: AsyncSession, room_id: uuid.UUID) -> bool: + room = await db.get(Room, room_id) + return room is not None and room.is_archived + + def _missing_scope(api_token: ApiToken | None, scope: str) -> bool: return api_token is not None and scope not in api_token.scopes @@ -186,6 +191,15 @@ async def chat_endpoint(websocket: WebSocket, db: AsyncSession = Depends(get_db) {"type": "error", "detail": "Not a member of this room"} ) continue + if await _is_room_archived(db, envelope.room_id): + # #57: history stays fully readable (joining/reading + # an archived room's channel is untouched above), + # this is the one gate that actually makes archiving + # do something for people who were already members. + await websocket.send_json( + {"type": "error", "detail": "This room has been archived and is read-only"} + ) + continue image_id = None if envelope.image_id is not None: image = await db.get(MessageImage, envelope.image_id) diff --git a/backend/tests/test_archive.py b/backend/tests/test_archive.py new file mode 100644 index 0000000..53cde72 --- /dev/null +++ b/backend/tests/test_archive.py @@ -0,0 +1,121 @@ +import uuid + +from app.models import User +from app.schemas.user import UserCreate +from app.services.auth_service import register_user +from tests.conftest import register_and_login + + +def _unique(prefix: str) -> str: + return f"{prefix}-{uuid.uuid4().hex[:8]}" + + +async def _make_admin(db_session, user_id: str) -> None: + user = await db_session.get(User, uuid.UUID(user_id)) + user.is_site_admin = True + await db_session.commit() + + +def _make_admin_ws(ws_client, user_id: str) -> None: + async def _promote(): + async with ws_client.session_factory() as session: + user = await session.get(User, uuid.UUID(user_id)) + user.is_site_admin = True + await session.commit() + + ws_client.portal.call(_promote) + + +def _register_ws(ws_client, username: str) -> dict: + async def _seed(): + async with ws_client.session_factory() as session: + await register_user( + session, + UserCreate(username=username, email=f"{username}@example.com", password="password123"), + ) + + ws_client.portal.call(_seed) + resp = ws_client.post( + "/api/auth/login", json={"username_or_email": username, "password": "password123"} + ) + assert resp.status_code == 200, resp.text + return resp.json() + + +async def test_archived_room_flag_reaches_existing_members(client, db_session): + # #57: is_archived used to only ever reach the admin portal's own + # AdminRoom schema -- a member's own view of the room (GET /rooms/mine) + # had no way to know it was archived at all. + admin = await register_and_login(client, db_session, username=_unique("admin")) + await _make_admin(db_session, admin["id"]) + room = (await client.post("/api/rooms", json={"name": _unique("general")})).json() + + resp = await client.post(f"/api/admin/rooms/{room['id']}/archive") + assert resp.status_code == 200 + + mine = (await client.get("/api/rooms/mine")).json() + entry = next(r for r in mine if r["id"] == room["id"]) + assert entry["is_archived"] is True + + # History stays fully readable for an existing member. + messages_resp = await client.get(f"/api/rooms/{room['id']}/messages") + assert messages_resp.status_code == 200 + + resp = await client.post(f"/api/admin/rooms/{room['id']}/unarchive") + assert resp.status_code == 200 + mine = (await client.get("/api/rooms/mine")).json() + entry = next(r for r in mine if r["id"] == room["id"]) + assert entry["is_archived"] is False + + +def test_ws_message_rejected_in_archived_room(ws_client_factory, db_session): + admin_ws = ws_client_factory() + admin = _register_ws(admin_ws, _unique("admin")) + _make_admin_ws(admin_ws, admin["id"]) + room = admin_ws.post("/api/rooms", json={"name": _unique("general")}).json() + + archive_resp = admin_ws.post(f"/api/admin/rooms/{room['id']}/archive") + assert archive_resp.status_code == 200 + + with admin_ws.websocket_connect("/ws/chat") as ws: + ws.send_json({"type": "join", "room_id": room["id"]}) + assert ws.receive_json()["type"] == "joined" + + ws.send_json({"type": "message", "room_id": room["id"], "content": "should not send"}) + received = ws.receive_json() + assert received["type"] == "error" + assert "archived" in received["detail"].lower() + + +def test_ws_message_allowed_again_after_unarchive(ws_client_factory): + admin_ws = ws_client_factory() + admin = _register_ws(admin_ws, _unique("admin")) + _make_admin_ws(admin_ws, admin["id"]) + room = admin_ws.post("/api/rooms", json={"name": _unique("general")}).json() + + admin_ws.post(f"/api/admin/rooms/{room['id']}/archive") + admin_ws.post(f"/api/admin/rooms/{room['id']}/unarchive") + + with admin_ws.websocket_connect("/ws/chat") as ws: + ws.send_json({"type": "join", "room_id": room["id"]}) + assert ws.receive_json()["type"] == "joined" + + ws.send_json({"type": "message", "room_id": room["id"], "content": "back online"}) + received = ws.receive_json() + assert received["type"] == "message" + assert received["content"] == "back online" + + +async def test_incoming_webhook_rejected_in_archived_room(client, db_session): + admin = await register_and_login(client, db_session, username=_unique("admin")) + await _make_admin(db_session, admin["id"]) + room = (await client.post("/api/rooms", json={"name": _unique("general")})).json() + webhook = (await client.post(f"/api/rooms/{room['id']}/webhooks/incoming", json={})).json() + + await client.post(f"/api/admin/rooms/{room['id']}/archive") + + resp = await client.post( + f"/api/webhooks/incoming/{webhook['token']}", json={"content": "should not post"} + ) + assert resp.status_code == 403 + assert "archived" in resp.json()["detail"].lower() diff --git a/frontend/src/components/ChatPane.tsx b/frontend/src/components/ChatPane.tsx index 73b8201..0a9dd71 100644 --- a/frontend/src/components/ChatPane.tsx +++ b/frontend/src/components/ChatPane.tsx @@ -272,6 +272,7 @@ export function ChatPane({ members={members} rooms={rooms} disabled={!connected} + archived={room.is_archived} onSend={send} /> diff --git a/frontend/src/components/Composer.tsx b/frontend/src/components/Composer.tsx index 58d32dd..45194fe 100644 --- a/frontend/src/components/Composer.tsx +++ b/frontend/src/components/Composer.tsx @@ -33,6 +33,12 @@ interface ComposerProps { // while typing and what actually renders as a link later agree. rooms: MyRoomItem[] disabled?: boolean + // #57: an archived room is permanently read-only, not just transiently + // disconnected -- kept as its own prop rather than folded into `disabled` + // so the placeholder/status text can say why, instead of the connecting/ + // offline copy below (which would be actively misleading here: waiting + // won't ever re-enable this). + archived?: boolean onSend: (content: string, imageId?: string, fileId?: string) => void } @@ -106,7 +112,12 @@ function AttachMenu({ onPickPhoto, onPickFile, onClose }: AttachMenuProps) { ) } -export function Composer({ roomId, roomName, isDm, members, rooms, disabled, onSend }: ComposerProps) { +export function Composer({ roomId, roomName, isDm, members, rooms, disabled, archived, onSend }: ComposerProps) { + // Every gate below (attach/emoji buttons, textarea, send button) reads + // this instead of the raw `disabled` prop -- an archived room must be + // just as unwritable as a disconnected one, it just says why differently + // (see the placeholder/status text further down). + const isDisabled = disabled || archived const [value, setValue] = useState('') const [pendingImage, setPendingImage] = useState<{ id: string; previewUrl: string } | null>(null) const [pendingFile, setPendingFile] = useState<{ id: string; filename: string; size: number } | null>( @@ -379,7 +390,7 @@ export function Composer({ roomId, roomName, isDm, members, rooms, disabled, onS function handleDragEnter(e: DragEvent) { e.preventDefault() - if (disabled) return + if (isDisabled) return dragCounterRef.current++ setDragActive(true) } @@ -401,7 +412,7 @@ export function Composer({ roomId, roomName, isDm, members, rooms, disabled, onS e.preventDefault() dragCounterRef.current = 0 setDragActive(false) - if (disabled) return + if (isDisabled) return // Only the first dropped file, matching the existing single-attachment- // per-message limit (the button-triggered file input isn't `multiple` // either). @@ -503,7 +514,7 @@ export function Composer({ roomId, roomName, isDm, members, rooms, disabled, onS type="button" className="composer-attach" onClick={() => setAttachMenuOpen((v) => !v)} - disabled={disabled || uploading} + disabled={isDisabled || uploading} aria-label="Attach a photo or file" aria-expanded={attachMenuOpen} > @@ -542,7 +553,7 @@ export function Composer({ roomId, roomName, isDm, members, rooms, disabled, onS type="button" className="composer-emoji-trigger" onClick={() => setEmojiPickerOpen((v) => !v)} - disabled={disabled} + disabled={isDisabled} aria-label="Insert an emoji" > 🙂 @@ -561,7 +572,7 @@ export function Composer({ roomId, roomName, isDm, members, rooms, disabled, onS ref={textareaRef} rows={1} value={value} - disabled={disabled} + disabled={isDisabled} onChange={(e) => { setValue(e.target.value) autoGrow() @@ -576,11 +587,13 @@ export function Composer({ roomId, roomName, isDm, members, rooms, disabled, onS onSelect={handleSelectionChange} onKeyDown={handleKeyDown} placeholder={ - disabled - ? online - ? 'Connecting…' - : "You're offline" - : `Message ${isDm ? roomName : `#${roomName}`}` + archived + ? 'This room has been archived' + : disabled + ? online + ? 'Connecting…' + : "You're offline" + : `Message ${isDm ? roomName : `#${roomName}`}` } spellCheck /> @@ -613,7 +626,7 @@ export function Composer({ roomId, roomName, isDm, members, rooms, disabled, onS type="button" className="composer-send" onClick={handleSend} - disabled={disabled || (!value.trim() && !pendingImage && !pendingFile)} + disabled={isDisabled || (!value.trim() && !pendingImage && !pendingFile)} aria-label="Send message" > - {disabled && ( -
{online ? 'Connecting…' : "You're offline — messages can't be sent right now"}
+ {archived ? ( +
This room has been archived and is read-only
+ ) : ( + disabled && ( +
{online ? 'Connecting…' : "You're offline — messages can't be sent right now"}
+ ) )} ) diff --git a/frontend/src/components/Sidebar.tsx b/frontend/src/components/Sidebar.tsx index f68aa55..84957fc 100644 --- a/frontend/src/components/Sidebar.tsx +++ b/frontend/src/components/Sidebar.tsx @@ -37,7 +37,11 @@ export function Sidebar({ } return room.name.toLowerCase().includes(query) } - const filtered = rooms.filter(matchesQuery) + // #57: archived rooms keep flowing through in `rooms` (so a member who + // still has one open via a direct link resolves fine -- see ChatPane), + // but they're a dead end going forward, so they don't belong in the list + // you'd browse/search from. + const filtered = rooms.filter((r) => !r.is_archived).filter(matchesQuery) const directMessages = filtered.filter((r) => r.is_dm) const regularRooms = filtered.filter((r) => !r.is_dm) diff --git a/frontend/src/types.ts b/frontend/src/types.ts index a0be121..2364587 100644 --- a/frontend/src/types.ts +++ b/frontend/src/types.ts @@ -57,6 +57,11 @@ export interface Room { description: string | null is_private: boolean is_dm: boolean + // #57: exposed here (not just the admin-only AdminRoom) so a member who + // still has this room -- direct link, or before their sidebar list next + // refreshes -- can be shown it's read-only instead of just silently + // failing to send. + is_archived: boolean owner_id: string created_at: string }