diff --git a/backend/alembic/versions/e2fc4d65f93e_allow_deleted_messages_to_clear_content_.py b/backend/alembic/versions/e2fc4d65f93e_allow_deleted_messages_to_clear_content_.py new file mode 100644 index 0000000..89293d4 --- /dev/null +++ b/backend/alembic/versions/e2fc4d65f93e_allow_deleted_messages_to_clear_content_.py @@ -0,0 +1,39 @@ +"""allow deleted messages to clear content and attachments + +Revision ID: e2fc4d65f93e +Revises: f3f255da9c96 +Create Date: 2026-08-28 16:20:02.114630 + +""" +from typing import Sequence, Union + +from alembic import op +import sqlalchemy as sa + + +# revision identifiers, used by Alembic. +revision: str = 'e2fc4d65f93e' +down_revision: Union[str, Sequence[str], None] = 'f3f255da9c96' +branch_labels: Union[str, Sequence[str], None] = None +depends_on: Union[str, Sequence[str], None] = None + + +def upgrade() -> None: + """Upgrade schema.""" + op.drop_constraint('messages_content_or_attachment_required', 'messages', type_='check') + op.create_check_constraint( + 'messages_content_or_attachment_required', + 'messages', + 'content IS NOT NULL OR image_id IS NOT NULL OR file_id IS NOT NULL ' + 'OR deleted_at IS NOT NULL', + ) + + +def downgrade() -> None: + """Downgrade schema.""" + op.drop_constraint('messages_content_or_attachment_required', 'messages', type_='check') + op.create_check_constraint( + 'messages_content_or_attachment_required', + 'messages', + 'content IS NOT NULL OR image_id IS NOT NULL OR file_id IS NOT NULL', + ) diff --git a/backend/app/models/message.py b/backend/app/models/message.py index 2503f49..a2eff86 100644 --- a/backend/app/models/message.py +++ b/backend/app/models/message.py @@ -10,8 +10,12 @@ from app.models.base import Base class Message(Base): __tablename__ = "messages" __table_args__ = ( + # #53: a deleted message clears content/image_id/file_id entirely + # (see message_service.delete_message) -- the "must have something" + # rule only applies while the message is actually live. CheckConstraint( - "content IS NOT NULL OR image_id IS NOT NULL OR file_id IS NOT NULL", + "content IS NOT NULL OR image_id IS NOT NULL OR file_id IS NOT NULL " + "OR deleted_at IS NOT NULL", name="messages_content_or_attachment_required", ), ) diff --git a/backend/app/routers/rooms.py b/backend/app/routers/rooms.py index 3c039d8..4e98b23 100644 --- a/backend/app/routers/rooms.py +++ b/backend/app/routers/rooms.py @@ -433,6 +433,7 @@ async def get_room_messages_endpoint( reactions=reactions_by_message.get(m.id, []), created_at=m.created_at, edited_at=m.edited_at, + deleted_at=m.deleted_at, ) for m in messages ] diff --git a/backend/app/schemas/message.py b/backend/app/schemas/message.py index 5733e58..a6f845e 100644 --- a/backend/app/schemas/message.py +++ b/backend/app/schemas/message.py @@ -44,3 +44,9 @@ class MessageRead(BaseModel): reactions: list[ReactionSummary] created_at: datetime edited_at: datetime | None + # #53: null for a live message; set once deleted, at which point + # content/image_id/file/link_preview are all already cleared + # server-side (see message_service.delete_message). `reactions` isn't + # cleared server-side -- the frontend just doesn't render them once + # deleted_at is set, same as it doesn't render the rest of a tombstone. + deleted_at: datetime | None diff --git a/backend/app/services/message_events.py b/backend/app/services/message_events.py index 671cd66..77d4744 100644 --- a/backend/app/services/message_events.py +++ b/backend/app/services/message_events.py @@ -134,6 +134,10 @@ async def _message_payload(db: AsyncSession, message: Message, username: str) -> "reactions": [], "created_at": message.created_at.isoformat(), "edited_at": message.edited_at.isoformat() if message.edited_at else None, + # Always null here -- a message just being created can't already be + # deleted -- but included for wire-format parity with MessageRead + # and message_deleted (#53). + "deleted_at": None, } @@ -204,6 +208,16 @@ async def broadcast_message_update( _maybe_fetch_link_preview(broadcaster, room_id, message) +async def broadcast_message_delete(broadcaster: Broadcaster, room_id: uuid.UUID, message_id: uuid.UUID) -> None: + # #53: no dispatch_event() call, deliberately -- same scope cut as + # broadcast_reaction_update's, and for the same reason (see + # backend/README.md): message.deleted isn't an outgoing-webhook event + # type here. + await broadcaster.publish( + room_id, {"type": "message_deleted", "id": str(message_id), "room_id": str(room_id)} + ) + + async def broadcast_reaction_update( broadcaster: Broadcaster, room_id: uuid.UUID, diff --git a/backend/app/services/message_service.py b/backend/app/services/message_service.py index a48cf76..441f632 100644 --- a/backend/app/services/message_service.py +++ b/backend/app/services/message_service.py @@ -6,11 +6,19 @@ from sqlalchemy import or_, select from sqlalchemy.ext.asyncio import AsyncSession from sqlalchemy.orm import selectinload -from app.models import Message, MessageMention, MessageReaction, MessageRoomReference +from app.models import ( + Message, + MessageFile, + MessageImage, + MessageMention, + MessageReaction, + MessageRoomReference, +) from app.schemas.message import ReactionSummary from app.services.link_preview_service import extract_first_url from app.services.mention_service import extract_mentioned_user_ids from app.services.room_reference_service import extract_referenced_room_ids +from app.storage import delete_file class MessageNotFoundError(Exception): @@ -56,7 +64,9 @@ async def edit_message( db: AsyncSession, message_id: uuid.UUID, editor_id: uuid.UUID, content: str ) -> Message: message = await db.get(Message, message_id) - if message is None: + # A deleted message might as well not exist for editing purposes -- + # same MessageNotFoundError a genuinely missing id would raise. + if message is None or message.deleted_at is not None: raise MessageNotFoundError() if message.user_id != editor_id: raise NotMessageAuthorError() @@ -69,6 +79,50 @@ async def edit_message( return message +async def delete_message(db: AsyncSession, message_id: uuid.UUID, deleter_id: uuid.UUID) -> Message: + message = await db.get(Message, message_id) + if message is None or message.deleted_at is not None: + raise MessageNotFoundError() + if message.user_id != deleter_id: + raise NotMessageAuthorError() + + # Fetch the attachment's storage filename (if any) before clearing the + # message's own FK to it -- the file is only unlinked from disk after a + # successful commit below, mirroring delete_room's identical ordering: + # a rolled-back transaction should never leave us having destroyed + # something we couldn't get back. + image_filename: str | None = None + file_filename: str | None = None + if message.image_id is not None: + image = await db.get(MessageImage, message.image_id) + if image is not None: + image_filename = image.storage_filename + await db.delete(image) + if message.file_id is not None: + message_file = await db.get(MessageFile, message.file_id) + if message_file is not None: + file_filename = message_file.storage_filename + await db.delete(message_file) + + # #53: a real delete, not just a UI hide -- content and any attachment + # are actually gone, not merely unlinked-but-still-fetchable. Only + # deleted_at (plus id/room_id/user_id/created_at, kept so the tombstone + # still occupies its place in history) survives. + message.content = None + message.image_id = None + message.file_id = None + message.preview_url = None + message.deleted_at = datetime.now(timezone.utc) + await db.commit() + await db.refresh(message) + + for filename in (image_filename, file_filename): + if filename is not None: + delete_file(filename) + + return message + + async def list_recent_messages( db: AsyncSession, room_id: uuid.UUID, limit: int = 50 ) -> list[Message]: diff --git a/backend/app/ws/chat.py b/backend/app/ws/chat.py index de403fa..6e77d97 100644 --- a/backend/app/ws/chat.py +++ b/backend/app/ws/chat.py @@ -11,6 +11,7 @@ from app.services.bot_service import resolve_token from app.services.message_events import ( broadcast_dm_presence_update, broadcast_member_updated, + broadcast_message_delete, broadcast_message_update, broadcast_new_message, broadcast_reaction_update, @@ -19,6 +20,7 @@ from app.services.message_service import ( MessageNotFoundError, NotMessageAuthorError, create_message, + delete_message, edit_message, toggle_reaction, ) @@ -261,6 +263,36 @@ async def chat_endpoint(websocket: WebSocket, db: AsyncSession = Depends(get_db) continue await broadcast_message_update(db, broadcaster, envelope.room_id, message) + elif envelope.type == "delete": + if envelope.room_id is None or envelope.message_id is None: + await websocket.send_json( + {"type": "error", "detail": "room_id and message_id required"} + ) + continue + if _missing_scope(api_token, "write:messages"): + await websocket.send_json( + {"type": "error", "detail": "Token missing required scope: write:messages"} + ) + continue + if envelope.room_id not in joined_rooms or not await _is_room_member( + db, envelope.room_id, user.id + ): + await websocket.send_json( + {"type": "error", "detail": "Not a member of this room"} + ) + continue + try: + await delete_message(db, envelope.message_id, user.id) + except MessageNotFoundError: + await websocket.send_json({"type": "error", "detail": "Message not found"}) + continue + except NotMessageAuthorError: + await websocket.send_json( + {"type": "error", "detail": "You can only delete your own messages"} + ) + continue + await broadcast_message_delete(broadcaster, envelope.room_id, envelope.message_id) + elif envelope.type == "reaction": if ( envelope.room_id is None @@ -285,7 +317,11 @@ async def chat_endpoint(websocket: WebSocket, db: AsyncSession = Depends(get_db) ) continue target_message = await db.get(Message, envelope.message_id) - if target_message is None or target_message.room_id != envelope.room_id: + if ( + target_message is None + or target_message.room_id != envelope.room_id + or target_message.deleted_at is not None + ): await websocket.send_json({"type": "error", "detail": "Message not found"}) continue reactions = await toggle_reaction(db, envelope.message_id, user.id, envelope.emoji) diff --git a/backend/tests/test_message_delete.py b/backend/tests/test_message_delete.py new file mode 100644 index 0000000..79c3864 --- /dev/null +++ b/backend/tests/test_message_delete.py @@ -0,0 +1,222 @@ +import io +import uuid + +from PIL import Image + +from app.schemas.user import UserCreate +from app.services.auth_service import register_user + + +def _unique(prefix: str) -> str: + return f"{prefix}-{uuid.uuid4().hex[:8]}" + + +_NOISE_TYPES = {"member_updated", "desktop_notification", "unread_update"} + + +def _recv(ws) -> dict: + """Reads the next frame, transparently discarding presence/offline- + notify noise -- see test_message_edit.py's identical helper.""" + while True: + msg = ws.receive_json() + if msg.get("type") not in _NOISE_TYPES: + return msg + + +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() + + +def _png_bytes() -> bytes: + buf = io.BytesIO() + Image.new("RGB", (10, 10), color=(255, 0, 0)).save(buf, format="PNG") + return buf.getvalue() + + +def test_ws_delete_clears_content_and_broadcasts(ws_client): + username = _unique("alice") + _register_ws(ws_client, username=username) + room = ws_client.post("/api/rooms", json={"name": _unique("general")}).json() + + with ws_client.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": "hello"}) + message = ws.receive_json() + + ws.send_json({"type": "delete", "room_id": room["id"], "message_id": message["id"]}) + deleted = ws.receive_json() + assert deleted == {"type": "message_deleted", "id": message["id"], "room_id": room["id"]} + + resp = ws_client.get(f"/api/rooms/{room['id']}/messages") + history = resp.json() + tombstone = next(m for m in history if m["id"] == message["id"]) + assert tombstone["content"] is None + assert tombstone["deleted_at"] is not None + assert tombstone["image_id"] is None + assert tombstone["file"] is None + + +def test_ws_delete_rejects_non_author(ws_client): + alice = _register_ws(ws_client, username=_unique("alice")) + room = ws_client.post("/api/rooms", json={"name": _unique("general")}).json() + + bob = _register_ws(ws_client, username=_unique("bob")) + ws_client.post(f"/api/rooms/{room['id']}/join") + + ws_client.post( + "/api/auth/login", json={"username_or_email": alice["username"], "password": "password123"} + ) + with ws_client.websocket_connect("/ws/chat") as alice_ws: + alice_ws.send_json({"type": "join", "room_id": room["id"]}) + assert alice_ws.receive_json()["type"] == "joined" + alice_ws.send_json({"type": "message", "room_id": room["id"], "content": "hello"}) + message = alice_ws.receive_json() + + ws_client.post( + "/api/auth/login", + json={"username_or_email": bob["username"], "password": "password123"}, + ) + with ws_client.websocket_connect("/ws/chat") as bob_ws: + bob_ws.send_json({"type": "join", "room_id": room["id"]}) + assert _recv(bob_ws)["type"] == "joined" + bob_ws.send_json( + {"type": "delete", "room_id": room["id"], "message_id": message["id"]} + ) + resp = bob_ws.receive_json() + assert resp["type"] == "error" + assert "own messages" in resp["detail"] + + resp = ws_client.get(f"/api/rooms/{room['id']}/messages") + history = resp.json() + still_there = next(m for m in history if m["id"] == message["id"]) + assert still_there["deleted_at"] is None + assert still_there["content"] == "hello" + + +def test_ws_delete_of_unknown_message_errors(ws_client): + _register_ws(ws_client, username=_unique("alice")) + room = ws_client.post("/api/rooms", json={"name": _unique("general")}).json() + + with ws_client.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": "delete", "room_id": room["id"], "message_id": str(uuid.uuid4())} + ) + resp = ws.receive_json() + assert resp == {"type": "error", "detail": "Message not found"} + + +def test_deleted_message_cannot_be_edited_or_reacted_to(ws_client): + _register_ws(ws_client, username=_unique("alice")) + room = ws_client.post("/api/rooms", json={"name": _unique("general")}).json() + + with ws_client.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": "hello"}) + message = ws.receive_json() + + ws.send_json({"type": "delete", "room_id": room["id"], "message_id": message["id"]}) + assert ws.receive_json()["type"] == "message_deleted" + + ws.send_json( + { + "type": "edit", + "room_id": room["id"], + "message_id": message["id"], + "content": "resurrected", + } + ) + assert ws.receive_json() == {"type": "error", "detail": "Message not found"} + + ws.send_json( + { + "type": "reaction", + "room_id": room["id"], + "message_id": message["id"], + "emoji": "👍", + } + ) + assert ws.receive_json() == {"type": "error", "detail": "Message not found"} + + +def test_delete_fans_out_across_instances(ws_client_factory): + instance1 = ws_client_factory() + instance2 = ws_client_factory() + + alice = _register_ws(instance1, _unique("alice")) + room = instance1.post("/api/rooms", json={"name": _unique("general")}).json() + + bob = _register_ws(instance2, _unique("bob")) + instance2.post(f"/api/rooms/{room['id']}/join") + + with instance2.websocket_connect("/ws/chat") as bob_ws: + bob_ws.send_json({"type": "join", "room_id": room["id"]}) + assert _recv(bob_ws)["type"] == "joined" + + with instance1.websocket_connect("/ws/chat") as alice_ws: + alice_ws.send_json({"type": "join", "room_id": room["id"]}) + assert alice_ws.receive_json()["type"] == "joined" + alice_ws.send_json({"type": "message", "room_id": room["id"], "content": "hi"}) + message = alice_ws.receive_json() + assert _recv(bob_ws)["type"] == "message" + + alice_ws.send_json( + {"type": "delete", "room_id": room["id"], "message_id": message["id"]} + ) + assert alice_ws.receive_json()["type"] == "message_deleted" + + deleted = _recv(bob_ws) + assert deleted == {"type": "message_deleted", "id": message["id"], "room_id": room["id"]} + + +def test_delete_removes_underlying_image_from_disk(ws_client): + username = _unique("alice") + _register_ws(ws_client, username=username) + room = ws_client.post("/api/rooms", json={"name": _unique("general")}).json() + + upload = ws_client.post( + f"/api/rooms/{room['id']}/images", + files={"file": ("test.png", _png_bytes(), "image/png")}, + ).json() + image_id = upload["id"] + + with ws_client.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"], "image_id": image_id}) + message = ws.receive_json() + assert message["image_id"] == image_id + + # Confirm the image actually serves before deleting, so a false + # pass (it was never reachable to begin with) can't hide as a true + # one below. + get_resp = ws_client.get(f"/api/rooms/{room['id']}/images/{image_id}") + assert get_resp.status_code == 200 + + ws.send_json({"type": "delete", "room_id": room["id"], "message_id": message["id"]}) + assert ws.receive_json()["type"] == "message_deleted" + + # The image is gone -- both the DB row (via the now-404ing serve + # endpoint) and, per #53's "delete the file too" choice, the file + # actually unlinked from disk (not just detached and orphaned). + get_resp = ws_client.get(f"/api/rooms/{room['id']}/images/{image_id}") + assert get_resp.status_code == 404 diff --git a/frontend/src/components/ChatPane.tsx b/frontend/src/components/ChatPane.tsx index 0a9dd71..68a525b 100644 --- a/frontend/src/components/ChatPane.tsx +++ b/frontend/src/components/ChatPane.tsx @@ -171,6 +171,21 @@ export function ChatPane({ setLive((prev) => prev.map((m) => (m.id === envelope.id ? { ...m, reactions: envelope.reactions } : m)), ) + } else if (envelope.type === 'message_deleted' && envelope.room_id === room.id) { + // Mirrors what the server already did to the row (see + // message_service.delete_message) -- content/image/file/preview + // cleared, deleted_at set. `reactions` is left alone; MessageList + // just doesn't render it once deleted_at is set, same as it + // doesn't render anything else here. + const tombstone = { + content: null, + image_id: null, + file: null, + link_preview: null, + deleted_at: new Date().toISOString(), + } + setHistory((prev) => prev.map((m) => (m.id === envelope.id ? { ...m, ...tombstone } : m))) + setLive((prev) => prev.map((m) => (m.id === envelope.id ? { ...m, ...tombstone } : m))) } else if (envelope.type === 'error') { setWsError(envelope.detail) } @@ -212,6 +227,10 @@ export function ChatPane({ (messageId: string, emoji: string) => socket.sendReaction(room.id, messageId, emoji), [socket, room.id], ) + const sendDelete = useCallback( + (messageId: string) => socket.sendDelete(room.id, messageId), + [socket, room.id], + ) return (
@@ -264,6 +283,7 @@ export function ChatPane({ myRooms={myRooms} onEdit={sendEdit} onReact={sendReaction} + onDelete={sendDelete} /> onEdit: (messageId: string, content: string) => void onReact: (messageId: string, emoji: string) => void + onDelete: (messageId: string) => void } -export function MessageList({ roomId, messages, members, myRooms, onEdit, onReact }: MessageListProps) { +export function MessageList({ + roomId, + messages, + members, + myRooms, + onEdit, + onReact, + onDelete, +}: MessageListProps) { const { user } = useAuth() const containerRef = useRef(null) const bottomRef = useRef(null) @@ -133,6 +142,14 @@ export function MessageList({ roomId, messages, members, myRooms, onEdit, onReac setEditingId(null) } + function handleDelete(messageId: string) { + // Matches the confirm() pattern already used for other destructive + // actions in this app (RoomInfoPanel's leave/delete-room, + // ProfileModal's delete-theme) rather than a custom dialog. + if (!confirm("Delete this message? This can't be undone.")) return + onDelete(messageId) + } + return (
{messages.map((msg, i) => { @@ -144,6 +161,7 @@ export function MessageList({ roomId, messages, members, myRooms, onEdit, onReac // applies uniformly, including to your own messages. const isGroupStart = !prev || prev.user_id !== msg.user_id const editing = editingId === msg.id + const deleted = !!msg.deleted_at return (
@@ -166,7 +184,11 @@ export function MessageList({ roomId, messages, members, myRooms, onEdit, onReac
)} - {editing ? ( + {deleted ? ( +
+ This message was deleted +
+ ) : editing ? (