From df6916385bc141ec13b792ba8c520a8a95898abf Mon Sep 17 00:00:00 2001 From: Igor Loskutov Date: Thu, 5 Feb 2026 20:30:26 -0500 Subject: [PATCH] fix: address review feedback - Add PUBLIC_MODE auth guard on bulk-status endpoint - Convert DB models to view models via model_validate() - Early return when no accessible rooms (skip DB queries) - BulkMeetingStatusMap: Partial for type honesty - Sort roomNames in query key for cache stability - Remove redundant empty-guard in queryFn - Add 7 backend tests: auth, redaction, whereby host_room_url, 401, empty - Add 2 frontend tests: error handling, unauthenticated case --- server/reflector/views/rooms.py | 19 +- server/tests/test_rooms_bulk_status.py | 184 ++++++++++++++++++ .../lib/__tests__/bulkMeetingStatus.test.tsx | 49 +++++ www/app/lib/apiHooks.ts | 8 +- 4 files changed, 253 insertions(+), 7 deletions(-) create mode 100644 server/tests/test_rooms_bulk_status.py diff --git a/server/reflector/views/rooms.py b/server/reflector/views/rooms.py index f461ee56..fde68a22 100644 --- a/server/reflector/views/rooms.py +++ b/server/reflector/views/rooms.py @@ -212,6 +212,9 @@ async def rooms_bulk_meeting_status( request: BulkStatusRequest, user: Annotated[Optional[auth.UserInfo], Depends(auth.current_user_optional)], ): + if not user and not settings.PUBLIC_MODE: + raise HTTPException(status_code=401, detail="Not authenticated") + user_id = user["sub"] if user else None all_rooms = await rooms_controller.get_by_names(request.room_names) @@ -224,13 +227,19 @@ async def rooms_bulk_meeting_status( room_by_id: dict[str, DbRoom] = {r.id: r for r in rooms} room_ids = list(room_by_id.keys()) + if not room_ids: + return { + name: RoomMeetingStatus(active_meetings=[], upcoming_events=[]) + for name in request.room_names + } + current_time = datetime.now(timezone.utc) active_meetings, upcoming_events = await asyncio.gather( meetings_controller.get_all_active_for_rooms(room_ids, current_time), calendar_events_controller.get_upcoming_for_rooms(room_ids), ) - # Group by room name + # Group by room name, converting DB models to view models active_by_room: dict[str, list[Meeting]] = defaultdict(list) for m in active_meetings: room = room_by_id.get(m.room_id) @@ -239,7 +248,9 @@ async def rooms_bulk_meeting_status( m.platform = room.platform if user_id != room.user_id and m.platform == "whereby": m.host_room_url = "" - active_by_room[room.name].append(m) + active_by_room[room.name].append( + Meeting.model_validate(m, from_attributes=True) + ) upcoming_by_room: dict[str, list[CalendarEventResponse]] = defaultdict(list) for e in upcoming_events: @@ -249,7 +260,9 @@ async def rooms_bulk_meeting_status( if user_id != room.user_id: e.description = None e.attendees = None - upcoming_by_room[room.name].append(e) + upcoming_by_room[room.name].append( + CalendarEventResponse.model_validate(e, from_attributes=True) + ) result: dict[str, RoomMeetingStatus] = {} for name in request.room_names: diff --git a/server/tests/test_rooms_bulk_status.py b/server/tests/test_rooms_bulk_status.py new file mode 100644 index 00000000..d6f8448c --- /dev/null +++ b/server/tests/test_rooms_bulk_status.py @@ -0,0 +1,184 @@ +from datetime import datetime, timedelta, timezone + +import pytest +from conftest import authenticated_client_ctx + +from reflector.db.calendar_events import CalendarEvent, calendar_events_controller +from reflector.db.meetings import meetings_controller +from reflector.db.rooms import Room, rooms_controller +from reflector.settings import settings + + +async def _create_room(name: str, user_id: str, is_shared: bool = False) -> Room: + return await rooms_controller.add( + name=name, + user_id=user_id, + zulip_auto_post=False, + zulip_stream="", + zulip_topic="", + is_locked=False, + room_mode="normal", + recording_type="cloud", + recording_trigger="automatic-2nd-participant", + is_shared=is_shared, + webhook_url="", + webhook_secret="", + ) + + +async def _create_meeting(room: Room, active: bool = True): + now = datetime.now(timezone.utc) + return await meetings_controller.create( + id=f"meeting-{room.name}-{now.timestamp()}", + room_name=room.name, + room_url="room-url", + host_room_url="host-url", + start_date=now - timedelta(minutes=10), + end_date=now + timedelta(minutes=50) if active else now - timedelta(minutes=1), + room=room, + ) + + +async def _create_calendar_event(room: Room): + now = datetime.now(timezone.utc) + return await calendar_events_controller.upsert( + CalendarEvent( + room_id=room.id, + ics_uid=f"event-{room.name}", + title=f"Upcoming in {room.name}", + description="secret description", + start_time=now + timedelta(minutes=30), + end_time=now + timedelta(minutes=90), + attendees=[{"name": "Alice", "email": "alice@example.com"}], + ) + ) + + +@pytest.mark.asyncio +async def test_bulk_status_returns_empty_for_no_rooms(client): + """Empty room_names returns empty dict.""" + async with authenticated_client_ctx(): + resp = await client.post("/rooms/meetings/bulk-status", json={"room_names": []}) + assert resp.status_code == 200 + assert resp.json() == {} + + +@pytest.mark.asyncio +async def test_bulk_status_returns_active_meetings_and_upcoming_events(client): + """Owner sees active meetings and upcoming events for their rooms.""" + room = await _create_room("bulk-test-room", "randomuserid") + await _create_meeting(room, active=True) + await _create_calendar_event(room) + + async with authenticated_client_ctx(): + resp = await client.post( + "/rooms/meetings/bulk-status", + json={"room_names": ["bulk-test-room"]}, + ) + + assert resp.status_code == 200 + data = resp.json() + assert "bulk-test-room" in data + status = data["bulk-test-room"] + assert len(status["active_meetings"]) == 1 + assert len(status["upcoming_events"]) == 1 + # Owner sees description + assert status["upcoming_events"][0]["description"] == "secret description" + + +@pytest.mark.asyncio +async def test_bulk_status_redacts_data_for_non_owner(client): + """Non-owner of a shared room gets redacted calendar events and no whereby host_room_url.""" + room = await _create_room("shared-bulk", "other-user-id", is_shared=True) + await _create_meeting(room, active=True) + await _create_calendar_event(room) + + # authenticated as "randomuserid" but room owned by "other-user-id" + async with authenticated_client_ctx(): + resp = await client.post( + "/rooms/meetings/bulk-status", + json={"room_names": ["shared-bulk"]}, + ) + + assert resp.status_code == 200 + status = resp.json()["shared-bulk"] + assert len(status["active_meetings"]) == 1 + assert len(status["upcoming_events"]) == 1 + # Non-owner: description and attendees redacted + assert status["upcoming_events"][0]["description"] is None + assert status["upcoming_events"][0]["attendees"] is None + + +@pytest.mark.asyncio +async def test_bulk_status_filters_private_rooms_of_other_users(client): + """User cannot see private rooms owned by others.""" + await _create_room("private-other", "other-user-id", is_shared=False) + + async with authenticated_client_ctx(): + resp = await client.post( + "/rooms/meetings/bulk-status", + json={"room_names": ["private-other"]}, + ) + + assert resp.status_code == 200 + status = resp.json()["private-other"] + assert status["active_meetings"] == [] + assert status["upcoming_events"] == [] + + +@pytest.mark.asyncio +async def test_bulk_status_redacts_whereby_host_room_url_for_non_owner(client): + """Non-owner of a shared whereby room gets empty host_room_url.""" + room = await _create_room("shared-whereby", "other-user-id", is_shared=True) + # Force platform to whereby + from reflector.db import get_database + from reflector.db.rooms import rooms as rooms_table + + await get_database().execute( + rooms_table.update() + .where(rooms_table.c.id == room.id) + .values(platform="whereby") + ) + + await _create_meeting(room, active=True) + + async with authenticated_client_ctx(): + resp = await client.post( + "/rooms/meetings/bulk-status", + json={"room_names": ["shared-whereby"]}, + ) + + assert resp.status_code == 200 + status = resp.json()["shared-whereby"] + assert len(status["active_meetings"]) == 1 + assert status["active_meetings"][0]["host_room_url"] == "" + + +@pytest.mark.asyncio +async def test_bulk_status_unauthenticated_rejected_non_public(client): + """Unauthenticated request on non-PUBLIC_MODE instance returns 401.""" + original = settings.PUBLIC_MODE + try: + settings.PUBLIC_MODE = False + resp = await client.post( + "/rooms/meetings/bulk-status", + json={"room_names": ["any-room"]}, + ) + assert resp.status_code == 401 + finally: + settings.PUBLIC_MODE = original + + +@pytest.mark.asyncio +async def test_bulk_status_nonexistent_room_returns_empty(client): + """Requesting a room that doesn't exist returns empty lists.""" + async with authenticated_client_ctx(): + resp = await client.post( + "/rooms/meetings/bulk-status", + json={"room_names": ["does-not-exist"]}, + ) + + assert resp.status_code == 200 + status = resp.json()["does-not-exist"] + assert status["active_meetings"] == [] + assert status["upcoming_events"] == [] diff --git a/www/app/lib/__tests__/bulkMeetingStatus.test.tsx b/www/app/lib/__tests__/bulkMeetingStatus.test.tsx index 72d23988..a0af4833 100644 --- a/www/app/lib/__tests__/bulkMeetingStatus.test.tsx +++ b/www/app/lib/__tests__/bulkMeetingStatus.test.tsx @@ -194,4 +194,53 @@ describe("bulk meeting status (prop-drilling)", () => { // No POST calls when no rooms expect(mockClient.POST).not.toHaveBeenCalled(); }); + + it("surfaces error when POST fails", async () => { + mockClient.POST.mockResolvedValue({ + data: undefined, + error: { detail: "server error" }, + response: {}, + }); + + function ErrorDisplay({ roomNames }: { roomNames: string[] }) { + const { error } = useRoomsBulkMeetingStatus(roomNames); + if (error) return
{error.message}
; + return
no error
; + } + + render(, { + wrapper: createWrapper(), + }); + + await waitFor(() => { + expect(screen.getByTestId("error")).toHaveTextContent( + "bulk-status fetch failed", + ); + }); + }); + + it("does not fetch when unauthenticated", async () => { + // Override useAuth to return unauthenticated + const authModule = jest.requireMock("../AuthProvider"); + const originalUseAuth = authModule.useAuth; + authModule.useAuth = () => ({ + ...originalUseAuth(), + status: "unauthenticated", + }); + + mockBulkStatusEndpoint(); + + render(, { + wrapper: createWrapper(), + }); + + await waitFor(() => { + expect(screen.getByTestId("status")).toHaveTextContent("no data"); + }); + + expect(mockClient.POST).not.toHaveBeenCalled(); + + // Restore + authModule.useAuth = originalUseAuth; + }); }); diff --git a/www/app/lib/apiHooks.ts b/www/app/lib/apiHooks.ts index 8e427943..1b3069ad 100644 --- a/www/app/lib/apiHooks.ts +++ b/www/app/lib/apiHooks.ts @@ -782,15 +782,15 @@ export function useRoomActiveMeetings(roomName: string | null) { type RoomMeetingStatus = components["schemas"]["RoomMeetingStatus"]; -export type BulkMeetingStatusMap = Record; +export type BulkMeetingStatusMap = Partial>; export function useRoomsBulkMeetingStatus(roomNames: string[]) { const { isAuthenticated } = useAuthReady(); + const sortedNames = [...roomNames].sort(); return useQuery({ - queryKey: ["bulk-meeting-status", roomNames], + queryKey: ["bulk-meeting-status", sortedNames], queryFn: async (): Promise => { - if (roomNames.length === 0) return {}; const { data, error } = await client.POST( "/v1/rooms/meetings/bulk-status", { body: { room_names: roomNames } }, @@ -802,7 +802,7 @@ export function useRoomsBulkMeetingStatus(roomNames: string[]) { } return data; }, - enabled: roomNames.length > 0 && isAuthenticated, + enabled: sortedNames.length > 0 && isAuthenticated, }); }