fix: soft-reset UX + stale-cookie handling + leaderboard 'is_you' by id
Three coupled fixes from the first manual test pass:
1. Stale signed cookie no longer 500s. `rooms.me()` now raises KeyError
when the participant row is gone (the previous code deref'd None and
threw TypeError, caught by quiz.js's catch-all as 'link expired').
`/api/session/{sid}/me` translates KeyError into 401 + delete_cookie,
so the client falls back to the join form cleanly.
Returning a JSONResponse directly because `raise HTTPException`
discards Response.delete_cookie mutations (FastAPI middleware
composes a fresh response on exception).
2. Reset is now a soft restart from the student's perspective. Before
closing each student WS in `RoomManager.reset`, the server now sends
a `{"type": "session_reset"}` message. The student SPA tears down
local state and re-runs boot(); /me returns 401 (now that the
participant is gone) and the join form renders without the user
having to manually reload. The WS close handler suppresses its
"Disconnected" screen during a reset to avoid a flash.
3. "You" highlight on the student leaderboard is now matched by id, not
by name. `RoomManager.leaderboard()` accepts an optional
`you_student_id` and stamps `is_you: true` on the matching entry only.
No other students' ids leak over the wire (we still don't include
`student_id` in the public top5 payload). quiz.js's renderBoard
prefers `r.is_you` when any row is marked, falling back to name match
for backward compatibility.
41/41 tests pass. Two new tests cover (a) the 401 + cookie-clear path
after reset and (b) `is_you` marking only the requesting student.
This commit is contained in:
39
app/room.py
39
app/room.py
@@ -130,7 +130,14 @@ class RoomManager:
|
|||||||
(sid,),
|
(sid,),
|
||||||
)
|
)
|
||||||
await db.commit()
|
await db.commit()
|
||||||
|
# Tell each student client the session was reset BEFORE closing the
|
||||||
|
# socket, so the JS can clear local state and re-bootstrap into the
|
||||||
|
# join form rather than showing a generic "disconnected" screen.
|
||||||
for ws in list(self.student_clients.get(sid, {}).keys()):
|
for ws in list(self.student_clients.get(sid, {}).keys()):
|
||||||
|
try:
|
||||||
|
await ws.send_json({"type": "session_reset"})
|
||||||
|
except Exception:
|
||||||
|
pass
|
||||||
try:
|
try:
|
||||||
await ws.close(code=4002)
|
await ws.close(code=4002)
|
||||||
except Exception:
|
except Exception:
|
||||||
@@ -504,13 +511,14 @@ class RoomManager:
|
|||||||
async def question_closed_message(self, sid: str, question_idx: int, identity: dict[str, Any] | None = None) -> dict[str, Any]:
|
async def question_closed_message(self, sid: str, question_idx: int, identity: dict[str, Any] | None = None) -> dict[str, Any]:
|
||||||
pool = await self.get_pool_for_session(sid)
|
pool = await self.get_pool_for_session(sid)
|
||||||
question = get_question(pool, question_idx)
|
question = get_question(pool, question_idx)
|
||||||
|
you_id = identity["student_id"] if identity else None
|
||||||
msg = {
|
msg = {
|
||||||
"type": "question_closed",
|
"type": "question_closed",
|
||||||
"question_idx": question_idx,
|
"question_idx": question_idx,
|
||||||
"correct": question["correct"],
|
"correct": question["correct"],
|
||||||
"explanation": question.get("explanation", ""),
|
"explanation": question.get("explanation", ""),
|
||||||
"histogram": await self.histogram(sid, question_idx),
|
"histogram": await self.histogram(sid, question_idx),
|
||||||
"top5": await self.leaderboard(sid, limit=5),
|
"top5": await self.leaderboard(sid, limit=5, you_student_id=you_id),
|
||||||
}
|
}
|
||||||
if identity:
|
if identity:
|
||||||
student = identity["student_id"]
|
student = identity["student_id"]
|
||||||
@@ -528,14 +536,16 @@ class RoomManager:
|
|||||||
return msg
|
return msg
|
||||||
|
|
||||||
async def between_message(self, sid: str, next_idx: int, identity: dict[str, Any] | None = None) -> dict[str, Any]:
|
async def between_message(self, sid: str, next_idx: int, identity: dict[str, Any] | None = None) -> dict[str, Any]:
|
||||||
msg = {"type": "between_questions", "next_idx": next_idx, "top5": await self.leaderboard(sid, limit=5)}
|
you_id = identity["student_id"] if identity else None
|
||||||
|
msg = {"type": "between_questions", "next_idx": next_idx, "top5": await self.leaderboard(sid, limit=5, you_student_id=you_id)}
|
||||||
if identity:
|
if identity:
|
||||||
msg["your_rank"] = await self.rank_for(sid, identity["student_id"])
|
msg["your_rank"] = await self.rank_for(sid, identity["student_id"])
|
||||||
msg["your_total"] = await self.total_for(sid, identity["student_id"])
|
msg["your_total"] = await self.total_for(sid, identity["student_id"])
|
||||||
return msg
|
return msg
|
||||||
|
|
||||||
async def ended_message(self, sid: str, identity: dict[str, Any] | None = None) -> dict[str, Any]:
|
async def ended_message(self, sid: str, identity: dict[str, Any] | None = None) -> dict[str, Any]:
|
||||||
msg = {"type": "session_ended", "final_top5": await self.leaderboard(sid, limit=5)}
|
you_id = identity["student_id"] if identity else None
|
||||||
|
msg = {"type": "session_ended", "final_top5": await self.leaderboard(sid, limit=5, you_student_id=you_id)}
|
||||||
if identity:
|
if identity:
|
||||||
student = identity["student_id"]
|
student = identity["student_id"]
|
||||||
msg.update(await self.student_summary(sid, student))
|
msg.update(await self.student_summary(sid, student))
|
||||||
@@ -564,7 +574,17 @@ class RoomManager:
|
|||||||
result["pending"] = max(0, int(total_row["count"]) - submitted - result["missed"])
|
result["pending"] = max(0, int(total_row["count"]) - submitted - result["missed"])
|
||||||
return result
|
return result
|
||||||
|
|
||||||
async def leaderboard(self, sid: str, limit: int | None = None, include_ids: bool = False) -> list[dict[str, Any]]:
|
async def leaderboard(
|
||||||
|
self,
|
||||||
|
sid: str,
|
||||||
|
limit: int | None = None,
|
||||||
|
include_ids: bool = False,
|
||||||
|
you_student_id: str | None = None,
|
||||||
|
) -> list[dict[str, Any]]:
|
||||||
|
"""Top scores. If `you_student_id` is given and that student appears
|
||||||
|
in the returned slice, that one entry is marked with `is_you: True`
|
||||||
|
so the client can highlight by id without exposing other students'
|
||||||
|
ids over the wire."""
|
||||||
query_limit = "" if limit is None else f"LIMIT {int(limit)}"
|
query_limit = "" if limit is None else f"LIMIT {int(limit)}"
|
||||||
async with connect(self.settings.db_path) as db:
|
async with connect(self.settings.db_path) as db:
|
||||||
cursor = await db.execute(
|
cursor = await db.execute(
|
||||||
@@ -585,6 +605,8 @@ class RoomManager:
|
|||||||
item = {"rank": rank, "name": row["name"], "score": int(row["score"])}
|
item = {"rank": rank, "name": row["name"], "score": int(row["score"])}
|
||||||
if include_ids:
|
if include_ids:
|
||||||
item["student_id"] = row["student_id"]
|
item["student_id"] = row["student_id"]
|
||||||
|
if you_student_id is not None and row["student_id"] == you_student_id:
|
||||||
|
item["is_you"] = True
|
||||||
board.append(item)
|
board.append(item)
|
||||||
return board
|
return board
|
||||||
|
|
||||||
@@ -656,6 +678,11 @@ class RoomManager:
|
|||||||
async with connect(self.settings.db_path) as db:
|
async with connect(self.settings.db_path) as db:
|
||||||
part_cursor = await db.execute("SELECT * FROM participants WHERE sid = ? AND student_id = ?", (sid, student_id))
|
part_cursor = await db.execute("SELECT * FROM participants WHERE sid = ? AND student_id = ?", (sid, student_id))
|
||||||
participant = await part_cursor.fetchone()
|
participant = await part_cursor.fetchone()
|
||||||
|
if participant is None:
|
||||||
|
# Participant row is gone (typically because the instructor
|
||||||
|
# ran a reset). Caller is expected to translate this into a
|
||||||
|
# 401 + cookie-clear so the client lands on the join form.
|
||||||
|
raise KeyError(f"No participant {student_id!r} in session {sid!r}")
|
||||||
sub_cursor = await db.execute(
|
sub_cursor = await db.execute(
|
||||||
"SELECT question_idx, answer, elapsed_ms, score, status FROM submissions WHERE sid = ? AND student_id = ? ORDER BY question_idx",
|
"SELECT question_idx, answer, elapsed_ms, score, status FROM submissions WHERE sid = ? AND student_id = ? ORDER BY question_idx",
|
||||||
(sid, student_id),
|
(sid, student_id),
|
||||||
@@ -677,7 +704,7 @@ class RoomManager:
|
|||||||
"response_time_avg_ms": None,
|
"response_time_avg_ms": None,
|
||||||
"response_time_distribution": {},
|
"response_time_distribution": {},
|
||||||
"average_score": 0,
|
"average_score": 0,
|
||||||
"top5": await self.leaderboard(sid, limit=5),
|
"top5": await self.leaderboard(sid, limit=5, you_student_id=student_id),
|
||||||
"your_rank": None,
|
"your_rank": None,
|
||||||
}
|
}
|
||||||
async with connect(self.settings.db_path) as db:
|
async with connect(self.settings.db_path) as db:
|
||||||
@@ -705,7 +732,7 @@ class RoomManager:
|
|||||||
"response_time_avg_ms": round(sum(times) / len(times)) if times else None,
|
"response_time_avg_ms": round(sum(times) / len(times)) if times else None,
|
||||||
"response_time_distribution": distribution,
|
"response_time_distribution": distribution,
|
||||||
"average_score": round(sum(scores) / len(scores), 2) if scores else 0,
|
"average_score": round(sum(scores) / len(scores), 2) if scores else 0,
|
||||||
"top5": await self.leaderboard(sid, limit=5),
|
"top5": await self.leaderboard(sid, limit=5, you_student_id=student_id),
|
||||||
}
|
}
|
||||||
if student_id:
|
if student_id:
|
||||||
payload["your_rank"] = await self.rank_for(sid, student_id)
|
payload["your_rank"] = await self.rank_for(sid, student_id)
|
||||||
|
|||||||
@@ -6,7 +6,7 @@ from pathlib import Path
|
|||||||
from uuid import uuid4
|
from uuid import uuid4
|
||||||
|
|
||||||
from fastapi import APIRouter, HTTPException, Request, Response, WebSocket
|
from fastapi import APIRouter, HTTPException, Request, Response, WebSocket
|
||||||
from fastapi.responses import FileResponse, HTMLResponse, RedirectResponse
|
from fastapi.responses import FileResponse, HTMLResponse, JSONResponse, RedirectResponse
|
||||||
|
|
||||||
from app import auth
|
from app import auth
|
||||||
from app.config import Settings
|
from app.config import Settings
|
||||||
@@ -70,7 +70,17 @@ def router(settings: Settings, rooms: RoomManager) -> APIRouter:
|
|||||||
identity = auth.get_student_identity(settings, request, sid)
|
identity = auth.get_student_identity(settings, request, sid)
|
||||||
if not identity:
|
if not identity:
|
||||||
raise HTTPException(status_code=401, detail="Student cookie required")
|
raise HTTPException(status_code=401, detail="Student cookie required")
|
||||||
|
try:
|
||||||
return await rooms.me(sid, identity["student_id"])
|
return await rooms.me(sid, identity["student_id"])
|
||||||
|
except KeyError:
|
||||||
|
# Cookie's student_id is no longer in the DB (e.g. session reset
|
||||||
|
# or DB rebuilt while the cookie persisted). Send 401 with the
|
||||||
|
# cookie cleared so the client renders the join form. We build
|
||||||
|
# the JSONResponse directly because raising HTTPException would
|
||||||
|
# bypass the cookie mutation.
|
||||||
|
resp = JSONResponse({"detail": "Re-join required"}, status_code=401)
|
||||||
|
resp.delete_cookie(auth.STUDENT_COOKIE, path="/")
|
||||||
|
return resp
|
||||||
|
|
||||||
@api.get("/api/session/{sid}/stats")
|
@api.get("/api/session/{sid}/stats")
|
||||||
async def stats(sid: str, request: Request, question_idx: int | None = None):
|
async def stats(sid: str, request: Request, question_idx: int | None = None):
|
||||||
|
|||||||
@@ -130,6 +130,10 @@ function connect() {
|
|||||||
try { handleMessage(JSON.parse(event.data)); } catch (e) { console.warn("bad ws msg", e); }
|
try { handleMessage(JSON.parse(event.data)); } catch (e) { console.warn("bad ws msg", e); }
|
||||||
});
|
});
|
||||||
ws.addEventListener("close", () => {
|
ws.addEventListener("close", () => {
|
||||||
|
// session_reset already drove a re-boot; suppress the generic
|
||||||
|
// "disconnected" screen so it doesn't briefly flash on top of the
|
||||||
|
// "Re-joining…" interstitial.
|
||||||
|
if (store.resetting) return;
|
||||||
stopCountdown();
|
stopCountdown();
|
||||||
setView(`
|
setView(`
|
||||||
<div class="card narrow center">
|
<div class="card narrow center">
|
||||||
@@ -149,10 +153,32 @@ function handleMessage(message) {
|
|||||||
case "question_closed": return renderReveal(message);
|
case "question_closed": return renderReveal(message);
|
||||||
case "between_questions": return renderBetween(message);
|
case "between_questions": return renderBetween(message);
|
||||||
case "session_ended": return renderFinished(message);
|
case "session_ended": return renderFinished(message);
|
||||||
|
case "session_reset": return handleSessionReset();
|
||||||
case "error": return renderError(message);
|
case "error": return renderError(message);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
function handleSessionReset() {
|
||||||
|
// Instructor cleared everyone out. Tear local state down and re-boot;
|
||||||
|
// /api/session/<sid>/me will return 401 (with cookie cleared by the
|
||||||
|
// server) and we'll land cleanly on the join form.
|
||||||
|
store.resetting = true;
|
||||||
|
stopCountdown();
|
||||||
|
store.me = null;
|
||||||
|
store.currentQuestion = null;
|
||||||
|
store.submitted = null;
|
||||||
|
store.pickedAnswer = null;
|
||||||
|
if (store.ws) { try { store.ws.close(); } catch {} store.ws = null; }
|
||||||
|
setView(`
|
||||||
|
<div class="card narrow center">
|
||||||
|
<h1>Session reset</h1>
|
||||||
|
<p class="muted">Your instructor reset the session. Re-joining…</p>
|
||||||
|
<div class="spinner" aria-hidden="true"></div>
|
||||||
|
</div>
|
||||||
|
`);
|
||||||
|
setTimeout(() => { store.resetting = false; boot(); }, 600);
|
||||||
|
}
|
||||||
|
|
||||||
function renderState(message) {
|
function renderState(message) {
|
||||||
store.currentQuestion = null;
|
store.currentQuestion = null;
|
||||||
store.submitted = null;
|
store.submitted = null;
|
||||||
@@ -314,15 +340,18 @@ function renderFinished(message) {
|
|||||||
|
|
||||||
function renderBoard(rows = []) {
|
function renderBoard(rows = []) {
|
||||||
if (!rows || !rows.length) return `<p class="muted small">No scores yet.</p>`;
|
if (!rows || !rows.length) return `<p class="muted small">No scores yet.</p>`;
|
||||||
// The server's student-facing top5 doesn't include student_id, so match
|
// The server marks the requesting student's row with `is_you: true` so
|
||||||
// on display name. Same-name collisions are rare in a single classroom
|
// we can highlight by id without other students' ids ever crossing the
|
||||||
// session; if it ever happens both rows highlight, which still reads
|
// wire. Falls back to name match only if the server didn't mark anything
|
||||||
// as "yours might be one of these" to the student.
|
// (older payloads pre-migration).
|
||||||
|
const anyMarked = rows.some((r) => r.is_you);
|
||||||
const myName = store.me?.name;
|
const myName = store.me?.name;
|
||||||
return `
|
return `
|
||||||
<ol class="leaderboard">
|
<ol class="leaderboard">
|
||||||
${rows.map((r) => {
|
${rows.map((r) => {
|
||||||
const isYou = myName && r.name && r.name === myName;
|
const isYou = anyMarked
|
||||||
|
? !!r.is_you
|
||||||
|
: (myName && r.name && r.name === myName);
|
||||||
return `
|
return `
|
||||||
<li class="${isYou ? "is-you" : ""}">
|
<li class="${isYou ? "is-you" : ""}">
|
||||||
<span class="rank">${r.rank}</span>
|
<span class="rank">${r.rank}</span>
|
||||||
|
|||||||
@@ -27,6 +27,48 @@ def test_root_without_sid_redirects_to_canonical(client, sid):
|
|||||||
assert response.headers["location"] == f"/?sid={sid}"
|
assert response.headers["location"] == f"/?sid={sid}"
|
||||||
|
|
||||||
|
|
||||||
|
def test_me_returns_401_and_clears_cookie_when_participant_is_gone(client, sid):
|
||||||
|
"""A stale signed cookie (e.g. after admin reset wiped participants) must
|
||||||
|
return 401 with the cookie cleared, not 500. The client uses 401 to fall
|
||||||
|
back to the join form."""
|
||||||
|
join_student(client, sid, "s1", "Student One")
|
||||||
|
assert client.get(f"/api/session/{sid}/me").status_code == 200
|
||||||
|
|
||||||
|
# Simulate the post-reset state: cookie still valid by signature,
|
||||||
|
# but the participant row is gone.
|
||||||
|
rooms = client.app.state.rooms
|
||||||
|
client.portal.call(rooms.reset, sid)
|
||||||
|
|
||||||
|
response = client.get(f"/api/session/{sid}/me")
|
||||||
|
assert response.status_code == 401
|
||||||
|
# Server should send a Set-Cookie that clears the qz_student cookie.
|
||||||
|
assert any(
|
||||||
|
h.lower() == "set-cookie" and "qz_student" in v and ('Max-Age=0' in v or 'expires=' in v.lower())
|
||||||
|
for h, v in response.headers.items()
|
||||||
|
), response.headers
|
||||||
|
|
||||||
|
|
||||||
|
def test_leaderboard_marks_requesting_student_with_is_you(client, sid):
|
||||||
|
"""The student-facing top5 should mark only the requesting student's row
|
||||||
|
with `is_you: true`, never include other students' ids."""
|
||||||
|
rooms = client.app.state.rooms
|
||||||
|
join_student(client, sid, "s1", "Alice")
|
||||||
|
join_student(client, sid, "s2", "Bob")
|
||||||
|
client.portal.call(rooms.open_question, sid, 0, 5)
|
||||||
|
client.portal.call(rooms.submit_answer, sid, "s1", 0, "B")
|
||||||
|
client.portal.call(rooms.submit_answer, sid, "s2", 0, "B")
|
||||||
|
client.portal.call(rooms.close_question, sid)
|
||||||
|
|
||||||
|
# Stats endpoint reflects the requesting student's identity from cookie.
|
||||||
|
stats = client.get(f"/api/session/{sid}/stats?question_idx=0").json()
|
||||||
|
you_rows = [r for r in stats["top5"] if r.get("is_you")]
|
||||||
|
other_rows = [r for r in stats["top5"] if not r.get("is_you")]
|
||||||
|
assert len(you_rows) == 1
|
||||||
|
assert you_rows[0]["name"] in {"Alice", "Bob"}
|
||||||
|
# Other students' ids are not exposed.
|
||||||
|
assert all("student_id" not in r for r in other_rows)
|
||||||
|
|
||||||
|
|
||||||
def test_invalid_session_and_missing_cookie_paths(client):
|
def test_invalid_session_and_missing_cookie_paths(client):
|
||||||
response = client.get("/?sid=BAD")
|
response = client.get("/?sid=BAD")
|
||||||
assert response.status_code == 404
|
assert response.status_code == 404
|
||||||
|
|||||||
Reference in New Issue
Block a user