amadan
room: hub-authorized removal naming, standing on takeover, fix round 1
Task 6 review, fix round 1 of 5 (branch head was b66bca1: Task 4's test round and Task 5's fix round already landed - getNav/boundedGet, Accept refusal is 406, signal limiter rekeyed, hub.Admit reordered, signalRaw). Important 1: room_test.go's negative cases used a second, empty harness, so a nonexistent meeting 404ed regardless of whether standing was ever checked. Rewrote following holding_test.go's C1-8 pattern: a real second meeting a guest holds no grant for (room, takeover confirm, takeover POST, all 404), an expired grant with no participation (404), and the positive fallback, an admitted guest surviving its own grant's expiry (200). TestEndingTheMeetingEndsTheRoom now also asserts the room 404s after the meeting ends. Important 2/3: added hub.PeerNameFor(sqid, session, target) - Remove's own authority (admitted member, target present and not the actor), read rather than acted on. confirmRemove now gates on it instead of standing alone, and its Title/Action can never name a peer the actor could not actually remove. Hub test covers all five cases from the brief. Important 4: confirmTakeover redirects to the holding screen (303) when this session holds no live participation - its "open in another tab" premise doesn't hold otherwise. Important 5: takeover POST re-derives standing fresh and refuses StandingNone before calling hub.Takeover; unlike the room page's and Remove's own authority, an admitted guest's expired grant does not survive this one (spec 2.5 asks for standing here, not just a participation). Rulings and minors: remove/takeover map the four hub sentinels to 404 via errors.Is and log-then-500 on anything else (signal.go's shape, item 7); the peer path segment is validated as 32 lowercase hex before any lookup (item 8, reusing signal.go's validPeerID - no behavioral difference observed since an unrecognised peer already 404s either way through the hub's own lookup; kept for the log-hygiene/no-unvalidated- input-past-the-gate consistency with signal.go, noted for the reviewer); confirm.html gained BackLabel (default "Meetings"), and the room's two confirm pages pass the meeting's title (item 9); AGENTS.md's UX-conventions line now cites the exception stated above it (item 10); a successful removal redirects to the holding screen, not the room (item 11); the holding page offers a takeover link with a sentence when this session already holds a peer here (item 12, holding gains OpenElsewhere); sseReader.nextOf now takes a time.Duration (item 13); room.html's own-standing sentence reads "joining as a", true before any participation exists (item 14); layout.html gained a head block and room.html's link/script moved into it (item 15). Every new or changed assertion was run against a reverted form of its own fix (confirmed to fail) and then against the fix (confirmed to pass); see task-6-report.md for the per-item log.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CzvnuKhLzudvq7jZ7SvVt2
12 files changed,
+379
−40
AGENTS.md+1 −1internal/meet/handlers_join.go+8 −1internal/meet/handlers_room.go+89 −20internal/meet/hub/hub.go+26 −0internal/meet/hub/hub_test.go+47 −0internal/meet/templates/confirm.html+1 −1internal/meet/templates/join.html+1 −0internal/meet/templates/layout.html+1 −0internal/meet/templates/room.html+4 −3internal/meettest/holding_test.go+20 −0internal/meettest/room_test.go+179 −12internal/meettest/sse_test.go+2 −2
CI passed — run details
diff --git a/AGENTS.md b/AGENTS.md| index 7467ca3..d6b731c 100644 |
| --- a/AGENTS.md |
| +++ b/AGENTS.md |
| @@ -66,7 +66,7 @@ by the framework, outside app middleware. |
| ## UX conventions (seeded from profile: considered) |
| -Accessible by construction, progressively enhanced, and never dependent on JavaScript to be usable. |
| +Accessible by construction, progressively enhanced, and never dependent on JavaScript to be usable, with the one exception named above: browser media participation, on the holding screen and in the room. |
| These are rastrillo's own conventions, and the profile is named for what it does rather than after anyone else's work. For wider reading on interface quality, impeccable.style, the WAI-ARIA Authoring Practices and Inclusive Components are all worth your time — none of them is endorsed by, or authoritative for, this framework. |
diff --git a/internal/meet/handlers_join.go b/internal/meet/handlers_join.go| index 185514e..499c642 100644 |
| --- a/internal/meet/handlers_join.go |
| +++ b/internal/meet/handlers_join.go |
| @@ -60,9 +60,16 @@ func (a *app) holding(w http.ResponseWriter, r *http.Request) { |
| if !ok { |
| return |
| } |
| + // This session's own peer, if it already has one here — a second |
| + // tab open on the same meeting — points the person at the takeover |
| + // confirm page rather than a plain join that would collide with it. |
| + var openElsewhere bool |
| + if session, ok := a.sessionKey(r); ok { |
| + _, openElsewhere = a.hub.LiveForSession(m.Sqid, session) |
| + } |
| render(w, "join", page(r, map[string]any{ |
| "Sqid": m.Sqid, "Title": a.store.title(m), "Starts": a.fmtStart(m.StartsAt), |
| "Name": p.Name, "Standing": st.String(), "IsMember": st == StandingMember, |
| - "Floor": !a.cfg.RelayConfigured, |
| + "Floor": !a.cfg.RelayConfigured, "OpenElsewhere": openElsewhere, |
| })) |
| } |
diff --git a/internal/meet/handlers_room.go b/internal/meet/handlers_room.go| index e178624..dc9bec5 100644 |
| --- a/internal/meet/handlers_room.go |
| +++ b/internal/meet/handlers_room.go |
| @@ -1,6 +1,7 @@ |
| package meet |
| import ( |
| + "errors" |
| "net/http" |
| "github.com/go-chi/chi/v5" |
| @@ -61,38 +62,67 @@ func (a *app) room(w http.ResponseWriter, r *http.Request) { |
| })) |
| } |
| +// hubRefusal is the four hub sentinels that all mean the same thing to |
| +// a caller of Remove or Takeover — this is not a peer, or not a |
| +// session, the caller has any business acting on — and all answer 404, |
| +// never 403 (spec §2.5). Anything else is unexpected and is a 500, |
| +// logged with its path and error only, signal.go's own shape for the |
| +// same distinction. |
| +func hubRefusal(err error) bool { |
| + return errors.Is(err, hub.ErrNotYours) || errors.Is(err, hub.ErrUnknownPeer) || |
| + errors.Is(err, hub.ErrNotAdmitted) || errors.Is(err, hub.ErrNotMember) |
| +} |
| + |
| // confirmRemove is GET /m/{sqid}/remove/{peer}: its own URL, per the |
| -// family's rule for a destructive action (spec §3.5). Standing member is |
| -// enough to see the page; hub.Remove is the actual authority, checked at |
| -// execution. |
| +// family's rule for a destructive action (spec §3.5). hub.PeerNameFor is |
| +// exactly Remove's own authority read rather than acted on, so this page |
| +// can never name — or offer to remove — a peer the actor could not |
| +// actually remove. |
| func (a *app) confirmRemove(w http.ResponseWriter, r *http.Request) { |
| - m, _, st, ok := a.roomAccess(w, r) |
| + m, _, _, ok := a.roomAccess(w, r) |
| if !ok { |
| return |
| } |
| - if st != StandingMember { |
| + target := chi.URLParam(r, "peer") |
| + if !validPeerID(hub.PeerID(target)) { |
| + ErrorPage(w, r, http.StatusNotFound, "") |
| + return |
| + } |
| + session, ok := a.sessionKey(r) |
| + if !ok { |
| + a.logger.Error("meet: confirm remove with no session subject", "path", r.URL.Path) |
| + ErrorPage(w, r, http.StatusInternalServerError, "") |
| + return |
| + } |
| + name, ok := a.hub.PeerNameFor(m.Sqid, session, hub.PeerID(target)) |
| + if !ok { |
| ErrorPage(w, r, http.StatusNotFound, "") |
| return |
| } |
| - target := chi.URLParam(r, "peer") |
| render(w, "confirm", page(r, map[string]any{ |
| - "Title": "Remove this participant?", |
| - "Body": "They will be disconnected from \"" + a.store.title(m) + "\" and can only come back through the meeting link.", |
| + "Title": "Remove " + name + " from the call?", |
| + // True for a guest, who has only the link, and for a member, who |
| + // can also come back from the meeting list — one sentence rather |
| + // than branching copy on the actor's own kind (noted for the |
| + // Task 11 copy catalogue). |
| + "Body": "They will be disconnected and can rejoin only if they are still allowed in.", |
| "Action": "/m/" + m.Sqid + "/remove/" + target, "Label": "Remove", |
| - "CancelHref": "/m/" + m.Sqid, "Sqid": m.Sqid, |
| + "CancelHref": "/m/" + m.Sqid, "BackLabel": a.store.title(m), "Sqid": m.Sqid, |
| })) |
| } |
| // remove is POST /m/{sqid}/remove/{peer}: hub.Remove is the only |
| -// authority (spec §3.5), re-derived at execution. Its ErrNotYours, |
| -// ErrUnknownPeer, ErrNotAdmitted and ErrNotMember all mean the same |
| -// thing to the caller — this is not a peer they may remove — and all |
| -// answer 404, never 403 (spec §2.5). |
| +// authority (spec §3.5), re-derived at execution. |
| func (a *app) remove(w http.ResponseWriter, r *http.Request) { |
| m, _, _, ok := a.roomAccess(w, r) |
| if !ok { |
| return |
| } |
| + target := chi.URLParam(r, "peer") |
| + if !validPeerID(hub.PeerID(target)) { |
| + ErrorPage(w, r, http.StatusNotFound, "") |
| + return |
| + } |
| session, ok := a.sessionKey(r) |
| if !ok { |
| a.logger.Error("meet: remove with no session subject", "path", r.URL.Path) |
| @@ -104,37 +134,71 @@ func (a *app) remove(w http.ResponseWriter, r *http.Request) { |
| ErrorPage(w, r, http.StatusNotFound, "") |
| return |
| } |
| - if err := a.hub.Remove(r.Context(), m.Sqid, session, actor, hub.PeerID(chi.URLParam(r, "peer"))); err != nil { |
| + if err := a.hub.Remove(r.Context(), m.Sqid, session, actor, hub.PeerID(target)); err != nil { |
| + if !hubRefusal(err) { |
| + a.logger.Error("meet: remove", "path", r.URL.Path, "err", err) |
| + ErrorPage(w, r, http.StatusInternalServerError, "") |
| + return |
| + } |
| ErrorPage(w, r, http.StatusNotFound, "") |
| return |
| } |
| - http.Redirect(w, r, "/m/"+m.Sqid, http.StatusSeeOther) |
| + // Not back to the room: that would open a fresh stream. The holding |
| + // screen is inert, and the actor is still there to open one anyway. |
| + http.Redirect(w, r, "/m/"+m.Sqid+"/join", http.StatusSeeOther) |
| } |
| // confirmTakeover is GET /m/{sqid}/takeover: its own URL, so a GET never |
| -// takes over (spec §4.2). roomAccess's standing-or-participation row |
| -// covers the page; hub.Takeover is the authority for the POST. |
| +// takes over (spec §4.2). Its premise — another tab has this session's |
| +// peer — only holds when this session actually has a live participation |
| +// here; without one there is nothing to confirm, so it sends the caller |
| +// straight to the holding screen instead of asking a question whose |
| +// answer would always be "there is no other tab." |
| func (a *app) confirmTakeover(w http.ResponseWriter, r *http.Request) { |
| m, _, _, ok := a.roomAccess(w, r) |
| if !ok { |
| return |
| } |
| + session, ok := a.sessionKey(r) |
| + if !ok { |
| + a.logger.Error("meet: confirm takeover with no session subject", "path", r.URL.Path) |
| + ErrorPage(w, r, http.StatusInternalServerError, "") |
| + return |
| + } |
| + if _, live := a.hub.LiveForSession(m.Sqid, session); !live { |
| + http.Redirect(w, r, "/m/"+m.Sqid+"/join", http.StatusSeeOther) |
| + return |
| + } |
| render(w, "confirm", page(r, map[string]any{ |
| "Title": "This meeting is open in another tab", |
| "Body": "Use this tab instead? The other tab will leave the call.", |
| "Action": "/m/" + m.Sqid + "/takeover", "Label": "Use this tab", |
| - "CancelHref": "/m/" + m.Sqid + "/join", "Sqid": m.Sqid, |
| + "CancelHref": "/m/" + m.Sqid + "/join", "BackLabel": a.store.title(m), "Sqid": m.Sqid, |
| })) |
| } |
| // takeover is POST /m/{sqid}/takeover: a confirmed action on the |
| // session's own live participation only (spec §4.2), never another |
| -// person's. |
| +// person's. §2.5 asks for standing as well as the participation — a |
| +// participation the room page and Remove's authority let survive a |
| +// lapsed grant (ruling 14) is not enough here on its own — so standing |
| +// is re-derived fresh rather than trusting roomAccess's page-view |
| +// fallback. |
| func (a *app) takeover(w http.ResponseWriter, r *http.Request) { |
| - m, _, _, ok := a.roomAccess(w, r) |
| + m, p, _, ok := a.roomAccess(w, r) |
| if !ok { |
| return |
| } |
| + st, err := a.standingFor(r.Context(), p, m) |
| + if err != nil { |
| + a.logger.Error("meet: takeover: standing", "err", err) |
| + ErrorPage(w, r, http.StatusInternalServerError, "") |
| + return |
| + } |
| + if st == StandingNone { |
| + ErrorPage(w, r, http.StatusNotFound, "") |
| + return |
| + } |
| session, ok := a.sessionKey(r) |
| if !ok { |
| a.logger.Error("meet: takeover with no session subject", "path", r.URL.Path) |
| @@ -142,6 +206,11 @@ func (a *app) takeover(w http.ResponseWriter, r *http.Request) { |
| return |
| } |
| if _, err := a.hub.Takeover(m.Sqid, session); err != nil { |
| + if !hubRefusal(err) { |
| + a.logger.Error("meet: takeover", "path", r.URL.Path, "err", err) |
| + ErrorPage(w, r, http.StatusInternalServerError, "") |
| + return |
| + } |
| ErrorPage(w, r, http.StatusNotFound, "") |
| return |
| } |
diff --git a/internal/meet/hub/hub.go b/internal/meet/hub/hub.go| index 6bb4ea5..c6e9a31 100644 |
| --- a/internal/meet/hub/hub.go |
| +++ b/internal/meet/hub/hub.go |
| @@ -235,6 +235,32 @@ func (h *Hub) LiveForSession(sqid, session string) (PeerID, bool) { |
| return id, ok |
| } |
| +// PeerNameFor answers target's display name exactly when the caller |
| +// (this session, in this room) could go on to remove it: an admitted |
| +// member participation, and target present in the same room and not |
| +// the caller's own. This is Remove's own authority (admission.go), |
| +// read here without mutating anything, so a confirm page can never |
| +// name — or offer to remove — a peer the caller could not actually |
| +// remove. |
| +func (h *Hub) PeerNameFor(sqid, session string, target PeerID) (string, bool) { |
| + h.mu.Lock() |
| + defer h.mu.Unlock() |
| + actor, ok := h.bySession[sqid][session] |
| + if !ok { |
| + return "", false |
| + } |
| + a, ok := h.findLocked(sqid, session, actor) |
| + if !ok || !a.Admitted || a.Kind != KindMember { |
| + return "", false |
| + } |
| + r := h.rooms[sqid] |
| + tp, ok := r.peers[target] |
| + if !ok || tp.Peer == a.Peer { |
| + return "", false |
| + } |
| + return tp.Name, true |
| +} |
| + |
| // Detach starts the resume window for a participation whose stream |
| // closed. Its subscriber is closed; a resume attaches a new one. |
| // |
diff --git a/internal/meet/hub/hub_test.go b/internal/meet/hub/hub_test.go| index d6d287e..59dee12 100644 |
| --- a/internal/meet/hub/hub_test.go |
| +++ b/internal/meet/hub/hub_test.go |
| @@ -172,3 +172,50 @@ func TestDetachOfADisplacedStreamLeavesTheReplacementAttached(t *testing.T) { |
| t.Fatalf("the attached stream's Detach did nothing: %+v", p) |
| } |
| } |
| + |
| +// PeerNameFor is Remove's own authority, read rather than acted on: it |
| +// must answer a name in exactly the cases Remove would go on to act, |
| +// and false everywhere else, so a confirm page can never name — or |
| +// offer to remove — a peer the caller could not actually remove. |
| +func TestPeerNameForIsExactlyRemovesAuthority(t *testing.T) { |
| + now := time.Date(2026, 9, 10, 9, 0, 0, 0, time.UTC) |
| + h := testHub(&now) |
| + ctx := context.Background() |
| + |
| + member, _ := h.Open("r1", "sMember", 1, "Ann", KindMember) |
| + if err := h.Join(ctx, "r1", "sMember", member.Peer); err != nil { |
| + t.Fatal(err) |
| + } |
| + target, _ := h.Open("r1", "sTarget", 2, "Bob", KindMember) |
| + |
| + if name, ok := h.PeerNameFor("r1", "sMember", target.Peer); !ok || name != "Bob" { |
| + t.Fatalf("member actor + present target: %q, %v", name, ok) |
| + } |
| + |
| + guest, _ := h.Open("r1", "sGuest", 100, "Cid", KindGuest) |
| + if err := h.Join(ctx, "r1", "sGuest", guest.Peer); err != nil { |
| + t.Fatal(err) |
| + } |
| + if p, ok := h.Find("r1", "sGuest", guest.Peer); !ok || p.Admitted { |
| + t.Fatal("the guest should be waiting, not admitted, for this case to test anything") |
| + } |
| + if _, ok := h.PeerNameFor("r1", "sGuest", target.Peer); ok { |
| + t.Fatal("a waiting guest is not admitted and must not name anyone") |
| + } |
| + |
| + other, _ := h.Open("r2", "sOther", 3, "Deb", KindMember) |
| + if err := h.Join(ctx, "r2", "sOther", other.Peer); err != nil { |
| + t.Fatal(err) |
| + } |
| + if _, ok := h.PeerNameFor("r1", "sMember", other.Peer); ok { |
| + t.Fatal("a target in another room must not be named") |
| + } |
| + |
| + if _, ok := h.PeerNameFor("r1", "sMember", member.Peer); ok { |
| + t.Fatal("the actor is not its own target") |
| + } |
| + |
| + if _, ok := h.PeerNameFor("r1", "sNobody", target.Peer); ok { |
| + t.Fatal("a session with no participation in this room must not name anyone") |
| + } |
| +} |
diff --git a/internal/meet/templates/confirm.html b/internal/meet/templates/confirm.html| index 7b7652c..9f53fec 100644 |
| --- a/internal/meet/templates/confirm.html |
| +++ b/internal/meet/templates/confirm.html |
| @@ -1,6 +1,6 @@ |
| {{define "title"}}{{.Title}}{{end}} |
| {{define "content"}} |
| -{{template "back-nav" dict "Href" .CancelHref "Label" "Meetings"}} |
| +{{template "back-nav" dict "Href" .CancelHref "Label" (or .BackLabel "Meetings")}} |
| {{template "page-header" dict "Title" .Title}} |
| <div rst-box> |
| <p>{{.Body}}</p> |
diff --git a/internal/meet/templates/join.html b/internal/meet/templates/join.html| index 59fd75c..5f3b0be 100644 |
| --- a/internal/meet/templates/join.html |
| +++ b/internal/meet/templates/join.html |
| @@ -5,6 +5,7 @@ |
| {{if .Starts}}<p>Starts {{.Starts}}.</p>{{end}} |
| <p>You are {{.Name}}, joining as a {{.Standing}}.</p> |
| {{if .Floor}}<p rst-notice>This team's Meet has no relay yet. Calls usually connect only between people on the same network.</p>{{end}} |
| +{{if .OpenElsewhere}}<p rst-notice>This meeting is already open in another tab. <a href="/m/{{.Sqid}}/takeover">Use this tab instead</a>.</p>{{end}} |
| <p>Calls coming soon</p> |
| <noscript><p>The call needs JavaScript to reach your camera and the other participants; everything else on this page works without it.</p></noscript> |
| <div id="devices" data-sqid="{{.Sqid}}" hidden></div> |
diff --git a/internal/meet/templates/layout.html b/internal/meet/templates/layout.html| index e42184d..333d81e 100644 |
| --- a/internal/meet/templates/layout.html |
| +++ b/internal/meet/templates/layout.html |
| @@ -9,6 +9,7 @@ |
| <script defer src="{{asset "static/rastrillo.js"}}"></script> |
| {{iconAssets}} |
| <script defer src="{{asset "static/select.js"}}"></script> |
| +{{block "head" .}}{{end}} |
| </head> |
| <body> |
| <div rst-shell-topbar> |
diff --git a/internal/meet/templates/room.html b/internal/meet/templates/room.html| index 408821d..8deaffe 100644 |
| --- a/internal/meet/templates/room.html |
| +++ b/internal/meet/templates/room.html |
| @@ -1,9 +1,12 @@ |
| {{define "title"}}{{.Title}}{{end}} |
| +{{define "head"}}<link rel="stylesheet" href="{{asset "static/meet.css"}}"> |
| +<script defer src="{{asset "static/room.js"}}"></script> |
| +{{end}} |
| {{define "content"}} |
| {{if .IsMember}}{{template "back-nav" dict "Href" "/" "Label" "Meetings"}}{{end}} |
| {{template "page-header" dict "Title" .Title}} |
| {{if .Starts}}<p>Starts {{.Starts}}.</p>{{end}} |
| -<p>You are {{.Name}}, in the call as a {{.Standing}}.</p> |
| +<p>You are {{.Name}}, joining as a {{.Standing}}.</p> |
| {{if .Floor}}<p rst-notice>This team's Meet has no relay yet. Calls usually connect only between people on the same network.</p>{{end}} |
| <noscript><p>The call needs JavaScript to reach your camera and the other participants; everything else on this page works without it. <a href="/m/{{.Sqid}}/join">Back to the holding screen</a>.</p></noscript> |
| <div id="room" data-sqid="{{.Sqid}}" data-boot="{{.Boot}}" data-self-name="{{.Name}}" data-is-member="{{if .IsMember}}1{{end}}" data-peer="" hidden> |
| @@ -17,6 +20,4 @@ |
| <p id="status" class="meet-status" role="status"></p> |
| </div> |
| {{if .IsMember}}<p><a href="/meetings/{{.Sqid}}/end" target="_blank" rel="noopener">End the meeting</a></p>{{end}} |
| -<link rel="stylesheet" href="{{asset "static/meet.css"}}"> |
| -<script defer src="{{asset "static/room.js"}}"></script> |
| {{end}} |
diff --git a/internal/meettest/holding_test.go b/internal/meettest/holding_test.go| index bf7041f..0f20ebf 100644 |
| --- a/internal/meettest/holding_test.go |
| +++ b/internal/meettest/holding_test.go |
| @@ -50,6 +50,26 @@ func TestHoldingPageIs404ForNoStanding(t *testing.T) { |
| } |
| } |
| +// Fix round 1, Important 12: once this session already holds a peer in |
| +// the room, the holding page points at the takeover confirm page rather |
| +// than offering a plain join that would collide with it. |
| +func TestHoldingPageOffersTakeoverWhenThisSessionIsAlreadyInTheRoom(t *testing.T) { |
| + x, sqid, _ := memberWithMeeting(t) |
| + m := x.browser(t) |
| + m.signIn("paul@example.com") |
| + if rec := m.get("/m/" + sqid + "/join"); strings.Contains(rec.Body.String(), "already open in another tab") { |
| + t.Fatal("holding page offers takeover before any stream is open") |
| + } |
| + s := m.stream("/m/" + sqid + "/stream") |
| + defer s.close() |
| + s.hello() |
| + rec := m.get("/m/" + sqid + "/join") |
| + body := rec.Body.String() |
| + if !strings.Contains(body, "already open in another tab") || !strings.Contains(body, "/m/"+sqid+"/takeover") { |
| + t.Fatalf("holding page lacks the takeover offer once a peer is live: %s", body) |
| + } |
| +} |
| + |
| func TestHoldingPageIs404ForEndedMeetings(t *testing.T) { |
| x, sqid, _ := memberWithMeeting(t) |
| m := x.browser(t) |
diff --git a/internal/meettest/room_test.go b/internal/meettest/room_test.go| index 5a31962..8b1a005 100644 |
| --- a/internal/meettest/room_test.go |
| +++ b/internal/meettest/room_test.go |
| @@ -5,6 +5,7 @@ import ( |
| "net/url" |
| "strings" |
| "testing" |
| + "time" |
| ) |
| func TestRoomPageIsScriptlessHonestAndStandingGated(t *testing.T) { |
| @@ -19,10 +20,90 @@ func TestRoomPageIsScriptlessHonestAndStandingGated(t *testing.T) { |
| if !strings.Contains(body, "<noscript>") || !strings.Contains(body, "needs JavaScript") { |
| t.Fatal("room page lacks its scriptless paragraph") |
| } |
| - stranger := newHarness(t).browser(t) |
| - stranger.signIn("nobody@example.com") |
| - if rec := stranger.get("/m/" + sqid); rec.Code != http.StatusNotFound { |
| - t.Fatalf("no standing: %d", rec.Code) |
| + // True before any participation exists: the room page renders |
| + // server-side, ahead of anything room.js does (Important 14). |
| + if !strings.Contains(body, "joining as a member") { |
| + t.Fatalf("room page's own-standing sentence: %s", body) |
| + } |
| + // meet.css and room.js move into layout.html's new head block |
| + // (Important 15): both must appear in <head>, ahead of </head>, |
| + // not at the tail of the body content. |
| + headEnd := strings.Index(body, "</head>") |
| + // asset() fingerprints the filename (static/meet.<hash>.css), so the |
| + // literal "meet.css" never appears — match the stable prefix instead. |
| + css := strings.Index(body, "/static/meet.") |
| + if headEnd < 0 || css < 0 || css > headEnd { |
| + t.Fatalf("meet.css is not inside <head>: head ends at %d, meet.css at %d\n%s", headEnd, css, body) |
| + } |
| + // The no-standing case belongs to |
| + // TestRoomAndTakeoverAreGatedByStandingOrParticipation: this |
| + // harness's member allowlist refuses to even mail a magic link to |
| + // an address with no standing and no pending join, so a "stranger" |
| + // here can only be tested against a real second meeting a |
| + // signed-in outsider holds no grant for — a fresh, memberless |
| + // harness would let anyone sign in and a nonexistent meeting would |
| + // 404 regardless of standing, which is exactly the vacuous version |
| + // of this test fix round 1's Important 1 replaced. |
| +} |
| + |
| +// review C1-8's pattern (holding_test.go), applied to the room and |
| +// takeover pages: a real second meeting the guest holds no grant for is |
| +// the test that actually exercises standing being checked, not merely |
| +// a meeting's absence answering 404 on its own (fix round 1, |
| +// Important 1). |
| +func TestRoomAndTakeoverAreGatedByStandingOrParticipation(t *testing.T) { |
| + x, first, tok := memberWithMeeting(t) |
| + member := x.browser(t) |
| + member.signIn("paul@example.com") |
| + second, _ := createMeeting(t, member, "A meeting they were not invited to", "") |
| + |
| + g := x.browser(t) |
| + g.get("/j/" + tok) |
| + g.signIn("guest@example.com") |
| + |
| + if rec := g.get("/m/" + second); rec.Code != http.StatusNotFound { |
| + t.Fatalf("room, a real meeting with no standing: %d", rec.Code) |
| + } |
| + if rec := g.get("/m/" + second + "/takeover"); rec.Code != http.StatusNotFound { |
| + t.Fatalf("takeover confirm, a real meeting with no standing: %d", rec.Code) |
| + } |
| + if rec := g.post("/m/"+second+"/takeover", nil); rec.Code != http.StatusNotFound { |
| + t.Fatalf("takeover POST, a real meeting with no standing: %d", rec.Code) |
| + } |
| + |
| + // The guest's own meeting, its grant now expired, and no |
| + // participation was ever opened for it: still 404. |
| + x.advance(2*time.Hour + time.Minute) |
| + if rec := g.get("/m/" + first); rec.Code != http.StatusNotFound { |
| + t.Fatalf("expired grant, no participation: %d", rec.Code) |
| + } |
| +} |
| + |
| +// The positive fallback for the same rule: an admitted guest survives |
| +// its own grant expiring (ruling 14, hub.standingLostLocked), because |
| +// the room page's authority is standing OR a live participation, and |
| +// the participation does not end when only the guest's grant does. |
| +func TestRoomPageSurvivesGrantExpiryForAnAdmittedGuest(t *testing.T) { |
| + x, sqid, tok := memberWithMeeting(t) |
| + m := x.browser(t) |
| + m.signIn("paul@example.com") |
| + sm := m.stream("/m/" + sqid + "/stream") |
| + defer sm.close() |
| + pm, _ := sm.hello() |
| + m.signal(sqid, pm, `{"type":"join"}`) |
| + |
| + g := x.browser(t) |
| + g.get("/j/" + tok) |
| + g.signIn("guest@example.com") |
| + sg := g.stream("/m/" + sqid + "/stream") |
| + defer sg.close() |
| + pg, _ := sg.hello() |
| + g.signal(sqid, pg, `{"type":"join"}`) |
| + m.signal(sqid, pm, `{"type":"admit","payload":{"peer":"`+pg+`"}}`) |
| + |
| + x.advance(2*time.Hour + time.Minute) |
| + if rec := g.get("/m/" + sqid); rec.Code != http.StatusOK { |
| + t.Fatalf("admitted guest past grant expiry: %d", rec.Code) |
| } |
| } |
| @@ -41,16 +122,49 @@ func TestRemoveNeedsAConfirmPageAndAnAdmittedMemberActor(t *testing.T) { |
| defer sg.close() |
| pg, _ := sg.hello() |
| g.signal(sqid, pg, `{"type":"join"}`) |
| - if rec := m.get("/m/" + sqid + "/remove/" + pg); rec.Code != http.StatusOK || !strings.Contains(rec.Body.String(), "Remove") { |
| - t.Fatalf("confirm page: %d", rec.Code) |
| + |
| + // The confirm page names the person, derived from hub.PeerNameFor — |
| + // exactly Remove's own authority, read rather than acted on |
| + // (Important 2, Important 3) — not merely the meeting. |
| + rec := m.get("/m/" + sqid + "/remove/" + pg) |
| + if rec.Code != http.StatusOK || !strings.Contains(rec.Body.String(), "Remove") || !strings.Contains(rec.Body.String(), "guest") { |
| + t.Fatalf("confirm page: %d\n%s", rec.Code, rec.Body.String()) |
| + } |
| + // Its back-nav names the meeting, not the generic "Meetings" list |
| + // this page's Cancel does not lead to (Important 9). |
| + if !strings.Contains(rec.Body.String(), "Board meeting") { |
| + t.Fatalf("confirm page back-nav: %s", rec.Body.String()) |
| + } |
| + // A peer that does not exist in the room at all must not render a |
| + // working confirm page: PeerNameFor answers nothing to remove. |
| + if rec := m.get("/m/" + sqid + "/remove/" + strings.Repeat("0", 32)); rec.Code != http.StatusNotFound { |
| + t.Fatalf("confirm page for an absent peer: %d", rec.Code) |
| + } |
| + // A path segment that is not the hub's own peer shape (32 lowercase |
| + // hex) is refused before it is ever looked up (Important 8), on |
| + // both the confirm page and the POST. |
| + if rec := m.get("/m/" + sqid + "/remove/not-a-peer-id"); rec.Code != http.StatusNotFound { |
| + t.Fatalf("malformed peer segment, confirm: %d", rec.Code) |
| + } |
| + if rec := m.post("/m/"+sqid+"/remove/not-a-peer-id", nil); rec.Code != http.StatusNotFound { |
| + t.Fatalf("malformed peer segment, POST: %d", rec.Code) |
| + } |
| + // A well-formed peer id that simply isn't in the room: hub.Remove's |
| + // ErrUnknownPeer, mapped to 404 by the sentinel check (Important 7), |
| + // not the 500 an unrecognised error would get. |
| + if rec := m.post("/m/"+sqid+"/remove/"+strings.Repeat("0", 32), nil); rec.Code != http.StatusNotFound { |
| + t.Fatalf("POST for an absent peer: %d", rec.Code) |
| } |
| if rec := g.post("/m/"+sqid+"/remove/"+pm, nil); rec.Code != http.StatusNotFound { |
| t.Fatalf("waiting guest removing the member: %d", rec.Code) |
| } |
| - if rec := m.post("/m/"+sqid+"/remove/"+pg, nil); rec.Code != http.StatusSeeOther { |
| - t.Fatalf("member removing: %d", rec.Code) |
| + // A successful removal lands on the holding screen, which opens no |
| + // stream of its own, not back on the room (Important 11). |
| + rec = m.post("/m/"+sqid+"/remove/"+pg, nil) |
| + if rec.Code != http.StatusSeeOther || rec.Header().Get("Location") != "/m/"+sqid+"/join" { |
| + t.Fatalf("member removing: %d %s", rec.Code, rec.Header().Get("Location")) |
| } |
| - ev, data, ok := sg.nextOf("lost", 2) |
| + ev, data, ok := sg.nextOf("lost", 2*time.Second) |
| if !ok || !strings.Contains(string(data), `"removed"`) { |
| t.Fatalf("removed guest got %s %s", ev, data) |
| } |
| @@ -73,9 +187,14 @@ func TestTakeoverIsAConfirmedPostAndAGetEndsNothing(t *testing.T) { |
| if rec := m.getStream("/m/" + sqid + "/stream?takeover=1"); rec.Code != http.StatusConflict { |
| t.Fatalf("a GET with a query parameter must not take over: %d", rec.Code) |
| } |
| - if rec := m.get("/m/" + sqid + "/takeover"); rec.Code != http.StatusOK || !strings.Contains(rec.Body.String(), "another tab") { |
| + rec := m.get("/m/" + sqid + "/takeover") |
| + if rec.Code != http.StatusOK || !strings.Contains(rec.Body.String(), "another tab") { |
| t.Fatalf("takeover confirm: %d", rec.Code) |
| } |
| + // Its back-nav names the meeting too (Important 9). |
| + if !strings.Contains(rec.Body.String(), "Board meeting") { |
| + t.Fatalf("takeover confirm back-nav: %s", rec.Body.String()) |
| + } |
| req := httptestPost("/m/"+sqid+"/takeover", url.Values{}) |
| req.Header.Set("Origin", "https://evil.example") |
| if rec := m.do(req); rec.Code != http.StatusForbidden { |
| @@ -84,7 +203,7 @@ func TestTakeoverIsAConfirmedPostAndAGetEndsNothing(t *testing.T) { |
| if rec := m.post("/m/"+sqid+"/takeover", nil); rec.Code != http.StatusSeeOther { |
| t.Fatalf("takeover: %d", rec.Code) |
| } |
| - if ev, data, ok := s1.nextOf("lost", 2); !ok || !strings.Contains(string(data), `"replaced"`) { |
| + if ev, data, ok := s1.nextOf("lost", 2*time.Second); !ok || !strings.Contains(string(data), `"replaced"`) { |
| t.Fatalf("old stream got %s %s", ev, data) |
| } |
| // getStream, not get, for the same reason as above: a resume is |
| @@ -100,6 +219,51 @@ func TestTakeoverIsAConfirmedPostAndAGetEndsNothing(t *testing.T) { |
| } |
| } |
| +// A confirm page's premise — "this is open in another tab" — only |
| +// holds when this session actually has a live participation to take |
| +// over; asked with nothing to take over, it sends the caller straight |
| +// to the holding screen instead (Important 4). |
| +func TestTakeoverConfirmRedirectsWhenThereIsNothingToTakeOver(t *testing.T) { |
| + x, sqid, _ := memberWithMeeting(t) |
| + m := x.browser(t) |
| + m.signIn("paul@example.com") |
| + rec := m.get("/m/" + sqid + "/takeover") |
| + if rec.Code != http.StatusSeeOther || rec.Header().Get("Location") != "/m/"+sqid+"/join" { |
| + t.Fatalf("takeover confirm with nothing to take over: %d %s", rec.Code, rec.Header().Get("Location")) |
| + } |
| +} |
| + |
| +// Takeover needs standing, not merely a participation (spec §2.5) — |
| +// unlike the room page's own authority, and unlike Remove's, both of |
| +// which an admitted guest's expired grant survives (ruling 14). |
| +// Important 5. |
| +func TestTakeoverRequiresStandingNotJustParticipation(t *testing.T) { |
| + x, sqid, tok := memberWithMeeting(t) |
| + m := x.browser(t) |
| + m.signIn("paul@example.com") |
| + sm := m.stream("/m/" + sqid + "/stream") |
| + defer sm.close() |
| + pm, _ := sm.hello() |
| + m.signal(sqid, pm, `{"type":"join"}`) |
| + |
| + g := x.browser(t) |
| + g.get("/j/" + tok) |
| + g.signIn("guest@example.com") |
| + sg := g.stream("/m/" + sqid + "/stream") |
| + defer sg.close() |
| + pg, _ := sg.hello() |
| + g.signal(sqid, pg, `{"type":"join"}`) |
| + m.signal(sqid, pm, `{"type":"admit","payload":{"peer":"`+pg+`"}}`) |
| + |
| + x.advance(2*time.Hour + time.Minute) |
| + if rec := g.get("/m/" + sqid); rec.Code != http.StatusOK { |
| + t.Fatalf("room page for the admitted guest: %d", rec.Code) |
| + } |
| + if rec := g.post("/m/"+sqid+"/takeover", nil); rec.Code != http.StatusNotFound { |
| + t.Fatalf("takeover past grant expiry: %d", rec.Code) |
| + } |
| +} |
| + |
| func TestEndingTheMeetingEndsTheRoom(t *testing.T) { |
| x, sqid, _ := memberWithMeeting(t) |
| m := x.browser(t) |
| @@ -109,7 +273,10 @@ func TestEndingTheMeetingEndsTheRoom(t *testing.T) { |
| p, _ := s.hello() |
| m.signal(sqid, p, `{"type":"join"}`) |
| m.post("/meetings/"+sqid+"/end", nil) |
| - if ev, data, ok := s.nextOf("lost", 2); !ok || !strings.Contains(string(data), `"ended"`) { |
| + if ev, data, ok := s.nextOf("lost", 2*time.Second); !ok || !strings.Contains(string(data), `"ended"`) { |
| t.Fatalf("got %s %s", ev, data) |
| } |
| + if rec := m.get("/m/" + sqid); rec.Code != http.StatusNotFound { |
| + t.Fatalf("room page after end: %d", rec.Code) |
| + } |
| } |
diff --git a/internal/meettest/sse_test.go b/internal/meettest/sse_test.go| index af43d30..3a21940 100644 |
| --- a/internal/meettest/sse_test.go |
| +++ b/internal/meettest/sse_test.go |
| @@ -154,9 +154,9 @@ func (s *sseReader) hello() (peer, boot string) { |
| // nextOf skips events until the named one arrives, or the timeout |
| // passes — for a test that expects some event (a roster, a peer) ahead |
| // of the one it actually wants to assert on. |
| -func (s *sseReader) nextOf(event string, timeoutSeconds int) (string, []byte, bool) { |
| +func (s *sseReader) nextOf(event string, timeout time.Duration) (string, []byte, bool) { |
| s.t.Helper() |
| - deadline := time.After(time.Duration(timeoutSeconds) * time.Second) |
| + deadline := time.After(timeout) |
| for { |
| select { |
| case ev := <-s.events: |