mirror of
https://github.com/Monadical-SAS/reflector.git
synced 2026-02-06 10:46:46 +00:00
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<Record> 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
This commit is contained in:
@@ -212,6 +212,9 @@ async def rooms_bulk_meeting_status(
|
|||||||
request: BulkStatusRequest,
|
request: BulkStatusRequest,
|
||||||
user: Annotated[Optional[auth.UserInfo], Depends(auth.current_user_optional)],
|
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
|
user_id = user["sub"] if user else None
|
||||||
|
|
||||||
all_rooms = await rooms_controller.get_by_names(request.room_names)
|
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_by_id: dict[str, DbRoom] = {r.id: r for r in rooms}
|
||||||
room_ids = list(room_by_id.keys())
|
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)
|
current_time = datetime.now(timezone.utc)
|
||||||
active_meetings, upcoming_events = await asyncio.gather(
|
active_meetings, upcoming_events = await asyncio.gather(
|
||||||
meetings_controller.get_all_active_for_rooms(room_ids, current_time),
|
meetings_controller.get_all_active_for_rooms(room_ids, current_time),
|
||||||
calendar_events_controller.get_upcoming_for_rooms(room_ids),
|
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)
|
active_by_room: dict[str, list[Meeting]] = defaultdict(list)
|
||||||
for m in active_meetings:
|
for m in active_meetings:
|
||||||
room = room_by_id.get(m.room_id)
|
room = room_by_id.get(m.room_id)
|
||||||
@@ -239,7 +248,9 @@ async def rooms_bulk_meeting_status(
|
|||||||
m.platform = room.platform
|
m.platform = room.platform
|
||||||
if user_id != room.user_id and m.platform == "whereby":
|
if user_id != room.user_id and m.platform == "whereby":
|
||||||
m.host_room_url = ""
|
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)
|
upcoming_by_room: dict[str, list[CalendarEventResponse]] = defaultdict(list)
|
||||||
for e in upcoming_events:
|
for e in upcoming_events:
|
||||||
@@ -249,7 +260,9 @@ async def rooms_bulk_meeting_status(
|
|||||||
if user_id != room.user_id:
|
if user_id != room.user_id:
|
||||||
e.description = None
|
e.description = None
|
||||||
e.attendees = 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] = {}
|
result: dict[str, RoomMeetingStatus] = {}
|
||||||
for name in request.room_names:
|
for name in request.room_names:
|
||||||
|
|||||||
184
server/tests/test_rooms_bulk_status.py
Normal file
184
server/tests/test_rooms_bulk_status.py
Normal file
@@ -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"] == []
|
||||||
@@ -194,4 +194,53 @@ describe("bulk meeting status (prop-drilling)", () => {
|
|||||||
// No POST calls when no rooms
|
// No POST calls when no rooms
|
||||||
expect(mockClient.POST).not.toHaveBeenCalled();
|
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 <div data-testid="error">{error.message}</div>;
|
||||||
|
return <div data-testid="error">no error</div>;
|
||||||
|
}
|
||||||
|
|
||||||
|
render(<ErrorDisplay roomNames={["room-x"]} />, {
|
||||||
|
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(<BulkStatusDisplay roomNames={["room-1"]} />, {
|
||||||
|
wrapper: createWrapper(),
|
||||||
|
});
|
||||||
|
|
||||||
|
await waitFor(() => {
|
||||||
|
expect(screen.getByTestId("status")).toHaveTextContent("no data");
|
||||||
|
});
|
||||||
|
|
||||||
|
expect(mockClient.POST).not.toHaveBeenCalled();
|
||||||
|
|
||||||
|
// Restore
|
||||||
|
authModule.useAuth = originalUseAuth;
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|||||||
@@ -782,15 +782,15 @@ export function useRoomActiveMeetings(roomName: string | null) {
|
|||||||
|
|
||||||
type RoomMeetingStatus = components["schemas"]["RoomMeetingStatus"];
|
type RoomMeetingStatus = components["schemas"]["RoomMeetingStatus"];
|
||||||
|
|
||||||
export type BulkMeetingStatusMap = Record<string, RoomMeetingStatus>;
|
export type BulkMeetingStatusMap = Partial<Record<string, RoomMeetingStatus>>;
|
||||||
|
|
||||||
export function useRoomsBulkMeetingStatus(roomNames: string[]) {
|
export function useRoomsBulkMeetingStatus(roomNames: string[]) {
|
||||||
const { isAuthenticated } = useAuthReady();
|
const { isAuthenticated } = useAuthReady();
|
||||||
|
const sortedNames = [...roomNames].sort();
|
||||||
|
|
||||||
return useQuery({
|
return useQuery({
|
||||||
queryKey: ["bulk-meeting-status", roomNames],
|
queryKey: ["bulk-meeting-status", sortedNames],
|
||||||
queryFn: async (): Promise<BulkMeetingStatusMap> => {
|
queryFn: async (): Promise<BulkMeetingStatusMap> => {
|
||||||
if (roomNames.length === 0) return {};
|
|
||||||
const { data, error } = await client.POST(
|
const { data, error } = await client.POST(
|
||||||
"/v1/rooms/meetings/bulk-status",
|
"/v1/rooms/meetings/bulk-status",
|
||||||
{ body: { room_names: roomNames } },
|
{ body: { room_names: roomNames } },
|
||||||
@@ -802,7 +802,7 @@ export function useRoomsBulkMeetingStatus(roomNames: string[]) {
|
|||||||
}
|
}
|
||||||
return data;
|
return data;
|
||||||
},
|
},
|
||||||
enabled: roomNames.length > 0 && isAuthenticated,
|
enabled: sortedNames.length > 0 && isAuthenticated,
|
||||||
});
|
});
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user