From bc2f365961e7c6f7ce32bb86fff1cf61a70a831f Mon Sep 17 00:00:00 2001 From: Cailyn Sinclair Date: Sun, 12 Jul 2026 20:52:51 -0700 Subject: [PATCH 1/6] Stabilize room host ownership Keep creator ownership separate from active admin handoffs and restore the creator deterministically on rejoin. Centralize role checks and document the authorization contract needed by room invitations.\n\nRefs #153 --- README.md | 3 + docs/room-ownership.md | 49 +++++++++++++ server/models/room.js | 36 ++++++---- server/models/room.test.js | 64 ++++++++++++++++- server/socket/lib/roomAuthorization.js | 10 ++- server/socket/lib/roomAuthorization.test.js | 40 ++++++++++- server/socket/namespaces/rooms.js | 12 ++-- server/socket/namespaces/rooms.test.js | 76 ++++++++++++++++++++- 8 files changed, 268 insertions(+), 22 deletions(-) create mode 100644 docs/room-ownership.md diff --git a/README.md b/README.md index 363d1e4..ed7b0c4 100644 --- a/README.md +++ b/README.md @@ -50,6 +50,9 @@ yarn start:client For more on the internals and contributing, check out the [wiki](https://github.com/coder13/LetsCube/wiki) +The server's owner/admin handoff and reconnect guarantees are documented in +[Room ownership and administration](docs/room-ownership.md). + ## Metrics The server stores pseudonymous room and authentication events in both the diff --git a/docs/room-ownership.md b/docs/room-ownership.md new file mode 100644 index 0000000..0016709 --- /dev/null +++ b/docs/room-ownership.md @@ -0,0 +1,49 @@ +# Room ownership and administration + +Rooms keep two separate host roles: + +- `owner` is the permanent creator. It does not change when the creator leaves, + and the owner may delete the room while absent. +- `admin` is the active participant who currently has room configuration and + moderation controls. + +The active admin is selected deterministically after a membership change: + +1. The owner is admin whenever they are in the room. +2. Otherwise, the existing admin remains in control while they are in the room. +3. If that admin leaves, the first active participant in the room's stored user + order takes control. +4. An empty room has no admin. Its owner remains unchanged, and a normal room + enters the existing stale-room expiry window. + +There is no user-facing manual transfer operation. If a future operation sets +an active participant as admin while the owner is absent, membership changes +preserve that transfer until the admin leaves or the owner returns. + +## Disconnect and rejoin behavior + +A transient disconnect does not immediately change membership or admin. The +owner or current admin keeps control during the configured reconnect grace +period. Reconnecting within that period cancels departure cleanup. After the +grace period, departure uses the same admin handoff as an explicit leave. + +Presence is per user, not per socket. Closing one tab does not hand off admin +while another socket for the same user remains in the room. Explicitly leaving +from the last tab finalizes the departure immediately. If the owner later +rejoins, they reclaim admin before the join is acknowledged. + +Grand Prix rooms use the same role selection when Grand Prix is enabled. When +it is disabled, create and join requests are rejected before membership or +roles change. + +## Authorization contract + +The server refreshes canonical room state before processing room socket events. +Admin actions must authorize against the current `admin`, not a client-provided +role or an older socket snapshot. Owner-only operations authorize against the +unchanging `owner`. + +Private-room invitations and access approvals may treat the active owner or +current admin as a host, but must also independently require that host to be an +active room participant. This prevents an absent owner from granting access +solely because owner deletion authority survives departure. diff --git a/server/models/room.js b/server/models/room.js index 88881ed..fef9c78 100644 --- a/server/models/room.js +++ b/server/models/room.js @@ -316,25 +316,34 @@ Room.methods.edit = async function (options) { return this.save(); }; -Room.methods.updateAdminIfNeeded = function (cb) { - if (this.usersInRoom.length === 0) { - this.admin = null; - return this.save(); +const sameUser = (left, right) => left && right + && String(left.id) === String(right.id); + +const selectRoomAdmin = ({ usersInRoom, owner, admin }) => { + if (usersInRoom.length === 0) { + return null; } - const findOwner = this.usersInRoom.find((user) => user.id === this.owner.id); - if (findOwner && this.admin && this.admin.id !== findOwner.id) { - this.admin = findOwner; - return this.save().then(cb); + const activeOwner = usersInRoom.find((user) => sameUser(user, owner)); + if (activeOwner) { + return activeOwner; } - if (!this.admin || this.admin.id !== this.usersInRoom[0].id) { - const { usersInRoom } = this; + return usersInRoom.find((user) => sameUser(user, admin)) || usersInRoom[0]; +}; + +Room.methods.updateAdminIfNeeded = async function (cb) { + const nextAdmin = selectRoomAdmin(this); + if (sameUser(this.admin, nextAdmin) || (!this.admin && !nextAdmin)) { + return this; + } - // eslint-disable-next-line prefer-destructuring - this.admin = usersInRoom[0]; - return this.save().then(cb); + this.admin = nextAdmin; + const room = await this.save(); + if (cb) { + cb(room); } + return room; }; const collectPostgresChanges = (room) => { @@ -411,3 +420,4 @@ Room.post('save', async (room) => { module.exports.Attempt = Attempt; module.exports.collectPostgresChanges = collectPostgresChanges; module.exports.Room = Room; +module.exports.selectRoomAdmin = selectRoomAdmin; diff --git a/server/models/room.test.js b/server/models/room.test.js index c955c76..752a519 100644 --- a/server/models/room.test.js +++ b/server/models/room.test.js @@ -4,7 +4,11 @@ const bcrypt = require('bcrypt'); const mongoose = require('mongoose'); const { generateScramble } = require('letscube-scrambles'); -const { collectPostgresChanges, Room } = require('./room'); +const { + collectPostgresChanges, + Room, + selectRoomAdmin, +} = require('./room'); jest.mock('letscube-scrambles', () => ({ generateScramble: jest.fn(), @@ -166,6 +170,64 @@ describe('room security helpers', () => { expect(room.expireAt.getTime()).toBeLessThanOrEqual(Date.now() + 10 * 60 * 1000); }); + it('restores the owner even when an empty room has no current admin', () => { + const owner = { id: 101 }; + const earlierParticipant = { id: 202 }; + + expect(selectRoomAdmin({ + usersInRoom: [earlierParticipant, owner], + owner, + admin: null, + })).toBe(owner); + }); + + it('keeps an active transferred admin while the owner is absent', () => { + const earlierParticipant = { id: 202 }; + const transferredAdmin = { id: 303 }; + + expect(selectRoomAdmin({ + usersInRoom: [earlierParticipant, transferredAdmin], + owner: { id: 101 }, + admin: transferredAdmin, + })).toBe(transferredAdmin); + }); + + it('promotes an active participant when both owner and admin are absent', () => { + const nextAdmin = { id: 303 }; + + expect(selectRoomAdmin({ + usersInRoom: [nextAdmin, { id: 404 }], + owner: { id: 101 }, + admin: { id: 202 }, + })).toBe(nextAdmin); + }); + + it('clears the admin when the room becomes empty', () => { + expect(selectRoomAdmin({ + usersInRoom: [], + owner: { id: 101 }, + admin: { id: 202 }, + })).toBeNull(); + }); + + it('persists and announces the owner reclaiming admin controls', async () => { + const owner = { id: 101 }; + const room = { + usersInRoom: [owner, { id: 202 }], + owner, + admin: { id: 202 }, + save: jest.fn(), + }; + room.save.mockResolvedValue(room); + const onAdminChange = jest.fn(); + + await Room.methods.updateAdminIfNeeded.call(room, onAdminChange); + + expect(room.admin).toBe(owner); + expect(room.save).toHaveBeenCalledTimes(1); + expect(onAdminChange).toHaveBeenCalledWith(room); + }); + it('stores an optional result submission id', () => { const room = RoomModel.hydrate({ _id: '507f1f77bcf86cd799439011', diff --git a/server/socket/lib/roomAuthorization.js b/server/socket/lib/roomAuthorization.js index dd9e278..b2a935a 100644 --- a/server/socket/lib/roomAuthorization.js +++ b/server/socket/lib/roomAuthorization.js @@ -1,5 +1,11 @@ const SUPER_ADMIN_ID = 8184; +const isRoomOwner = (userId, room) => !!userId && !!room && !!room.owner + && +room.owner.id === +userId; + +const isRoomAdmin = (userId, room) => !!userId && !!room && !!room.admin + && +room.admin.id === +userId; + const canDeleteRoom = (userId, room) => { if (+userId === SUPER_ADMIN_ID) { return true; @@ -9,7 +15,7 @@ const canDeleteRoom = (userId, room) => { return false; } - return [room.owner, room.admin].some((user) => user && +user.id === +userId); + return isRoomOwner(userId, room) || isRoomAdmin(userId, room); }; const canAccessRoom = (userId, room) => { @@ -28,4 +34,6 @@ const canAccessRoom = (userId, room) => { module.exports = { canAccessRoom, canDeleteRoom, + isRoomAdmin, + isRoomOwner, }; diff --git a/server/socket/lib/roomAuthorization.test.js b/server/socket/lib/roomAuthorization.test.js index e879047..622c213 100644 --- a/server/socket/lib/roomAuthorization.test.js +++ b/server/socket/lib/roomAuthorization.test.js @@ -1,6 +1,31 @@ /* eslint-env jest */ -const { canAccessRoom, canDeleteRoom } = require('./roomAuthorization'); +const { + canAccessRoom, + canDeleteRoom, + isRoomAdmin, + isRoomOwner, +} = require('./roomAuthorization'); + +describe('room roles', () => { + const room = { + owner: { id: 101 }, + admin: { id: 202 }, + }; + + it('distinguishes the permanent owner from the current admin', () => { + expect(isRoomOwner('101', room)).toBe(true); + expect(isRoomOwner(202, room)).toBe(false); + expect(isRoomAdmin('202', room)).toBe(true); + expect(isRoomAdmin(101, room)).toBe(false); + }); + + it('rejects missing users, rooms, and role holders', () => { + expect(isRoomOwner(null, room)).toBe(false); + expect(isRoomOwner(101, null)).toBe(false); + expect(isRoomAdmin(202, { ...room, admin: null })).toBe(false); + }); +}); describe('canDeleteRoom', () => { const room = { @@ -21,6 +46,19 @@ describe('canDeleteRoom', () => { it('allows the global administrator', () => { expect(canDeleteRoom(8184, null)).toBe(true); }); + + it('keeps absent-owner deletion separate from active host authority', () => { + const roomWithAbsentOwner = { + owner: { id: 101 }, + admin: { id: 202 }, + inRoom: new Map([['101', false], ['202', true]]), + banned: new Map(), + }; + + expect(canDeleteRoom(101, roomWithAbsentOwner)).toBe(true); + expect(canAccessRoom(101, roomWithAbsentOwner)).toBe(false); + expect(isRoomAdmin(101, roomWithAbsentOwner)).toBe(false); + }); }); describe('canAccessRoom', () => { diff --git a/server/socket/namespaces/rooms.js b/server/socket/namespaces/rooms.js index 338de97..2e0cc9c 100644 --- a/server/socket/namespaces/rooms.js +++ b/server/socket/namespaces/rooms.js @@ -10,7 +10,11 @@ const { markRoomDeleted } = require('../../postgres/dualWrite'); const { Room, User } = require('../../models'); const { encodeUserRoom } = require('../utils'); const roomMap = require('../lib/roomMap'); -const { canAccessRoom, canDeleteRoom } = require('../lib/roomAuthorization'); +const { + canAccessRoom, + canDeleteRoom, + isRoomAdmin, +} = require('../lib/roomAuthorization'); const { isRoomTypeEnabled: checkRoomTypeEnabled } = require('../lib/roomAvailability'); const { removeUserFromRoomSockets } = require('../lib/roomSockets'); const { createReconnectGrace } = require('../lib/reconnectGrace'); @@ -323,7 +327,7 @@ module.exports = (io, middlewares) => { if (!isLoggedIn() || !isInRoom()) { logger.debug('Unauthenticated user or user not in room attempting to perform admin action'); return false; - } if (socket.room.admin.id !== socket.user.id) { + } if (!isRoomAdmin(socket.user.id, socket.room)) { logger.debug('Non-admin attempting to perform admin action'); socket.emit(Protocol.ERROR, { statusCode: 403, @@ -756,7 +760,7 @@ module.exports = (io, middlewares) => { } const { userId } = result; - if (userId !== socket.user.id && socket.user.id !== socket.room.admin.id) { + if (userId !== socket.user.id && !isRoomAdmin(socket.user.id, socket.room)) { socket.emit(Protocol.ERROR, { statusCode: 400, event: Protocol.SEND_EDIT_RESULT, @@ -843,7 +847,7 @@ module.exports = (io, middlewares) => { }); } - if (socket.room.admin.id !== socket.user.id) { + if (!isRoomAdmin(socket.user.id, socket.room)) { logger.debug('Non-admin attempting to edit a room'); return rejectEdit({ statusCode: 403, diff --git a/server/socket/namespaces/rooms.test.js b/server/socket/namespaces/rooms.test.js index e24cb4c..b153e5d 100644 --- a/server/socket/namespaces/rooms.test.js +++ b/server/socket/namespaces/rooms.test.js @@ -103,13 +103,14 @@ const makeSocket = ({ }; const setup = (rooms) => { + const roomChannel = { emit: jest.fn() }; const namespace = { adapter: { allRooms: jest.fn().mockResolvedValue(new Set()), sockets: jest.fn().mockResolvedValue(new Set()), }, emit: jest.fn(), - in: jest.fn(() => ({ emit: jest.fn() })), + in: jest.fn(() => roomChannel), on: jest.fn(), use: jest.fn(), }; @@ -125,10 +126,13 @@ const setup = (rooms) => { User.find.mockResolvedValue([]); initRooms(io, []); - return async (socket) => { + const connectSocket = async (socket) => { await connect(socket); return socket; }; + connectSocket.namespace = namespace; + connectSocket.roomChannel = roomChannel; + return connectSocket; }; const joinRoom = async (socket, payload) => { @@ -206,6 +210,74 @@ describe('private room namespace joins', () => { expect.objectContaining({ _id: otherRoom._id }), ); }); + + it('returns restored controls when the owner rejoins after an admin handoff', async () => { + const owner = { id: 101 }; + const room = makeRoom({ + private: false, + password: null, + owner, + admin: { id: 202 }, + inRoom: new Map([['101', false], ['202', true]]), + }); + room.addUser.mockImplementation(async (user, spectating, onAdminChange) => { + room.inRoom.set(user.id.toString(), true); + room.admin = user; + onAdminChange(room); + return room; + }); + room.edit = jest.fn().mockResolvedValue(room); + const connect = setup(new Map([[room._id, room]])); + const socket = await connect(makeSocket({ + id: 'owner-socket', + session: {}, + userId: owner.id, + })); + + const joinAcknowledgment = await joinRoom(socket, { id: room._id }); + + expect(joinAcknowledgment).toHaveBeenCalledWith( + null, + expect.objectContaining({ admin: expect.objectContaining({ id: owner.id }) }), + ); + expect(connect.roomChannel.emit).toHaveBeenCalledWith( + Protocol.UPDATE_ADMIN, + expect.objectContaining({ id: owner.id }), + ); + + const editAcknowledgment = jest.fn(); + await socket.handlers[Protocol.EDIT_ROOM]({ + name: 'Owner is back', + private: false, + type: 'normal', + }, editAcknowledgment); + + expect(room.edit).toHaveBeenCalledTimes(1); + expect(editAcknowledgment).toHaveBeenCalledWith( + null, + expect.objectContaining({ admin: expect.objectContaining({ id: owner.id }) }), + ); + }); + + it('does not change ownership when a disabled Grand Prix room is joined', async () => { + const room = makeRoom({ + type: 'grand_prix', + private: false, + password: null, + admin: { id: 202 }, + }); + const connect = setup(new Map([[room._id, room]])); + const socket = await connect(makeSocket({ id: 'grand-prix-socket', session: {} })); + + const acknowledgment = await joinRoom(socket, { id: room._id }); + + expect(acknowledgment).toHaveBeenCalledWith( + expect.objectContaining({ reason: 'grand_prix_disabled' }), + expect.objectContaining({ _id: room._id }), + ); + expect(room.addUser).not.toHaveBeenCalled(); + expect(room.admin).toEqual({ id: 202 }); + }); }); describe('room namespace edits', () => { From b8767591b9f2770bb3b7e722ed01695c6c7bbf57 Mon Sep 17 00:00:00 2001 From: Cailyn Sinclair Date: Sun, 12 Jul 2026 21:19:50 -0700 Subject: [PATCH 2/6] Harden room departure handoffs Preserve the non-null admin socket contract, always select and persist a successor on removal, and reject self-removal moderation requests. Make explicit two-tab leaves remove their own socket membership before checking for remaining tabs so the last departure cannot strand a room.\n\nRefs #153 --- docs/room-ownership.md | 12 ++- server/models/room.js | 8 +- server/models/room.test.js | 50 ++++++++++++ server/socket/namespaces/rooms.js | 37 +++++++-- server/socket/namespaces/rooms.test.js | 108 ++++++++++++++++++++++++- 5 files changed, 202 insertions(+), 13 deletions(-) diff --git a/docs/room-ownership.md b/docs/room-ownership.md index 0016709..48e5131 100644 --- a/docs/room-ownership.md +++ b/docs/room-ownership.md @@ -29,9 +29,15 @@ grace period, departure uses the same admin handoff as an explicit leave. Presence is per user, not per socket. Closing one tab does not hand off admin while another socket for the same user remains in the room. Explicitly leaving -from the last tab finalizes the departure immediately. If the owner later +from the last tab finalizes the departure immediately: each leaving tab removes +itself from the per-user socket group before the remaining-tab check, so two +simultaneous explicit leaves cannot strand membership. If the owner later rejoins, they reclaim admin before the join is acknowledged. +An empty room persists `admin: null`, but the established `UPDATE_ADMIN` +Socket.IO event is emitted only for a non-null active admin. Spectators and +other clients therefore never receive a null admin payload. + Grand Prix rooms use the same role selection when Grand Prix is enabled. When it is disabled, create and join requests are rejected before membership or roles change. @@ -43,6 +49,10 @@ Admin actions must authorize against the current `admin`, not a client-provided role or an older socket snapshot. Owner-only operations authorize against the unchanging `owner`. +Admins cannot kick or ban themselves through raw socket events; they must use +the ordinary leave operation so membership, admin handoff, and reconnect +semantics remain coherent. + Private-room invitations and access approvals may treat the active owner or current admin as a host, but must also independently require that host to be an active room participant. This prevents an absent owner from granting access diff --git a/server/models/room.js b/server/models/room.js index fef9c78..bc4bdc3 100644 --- a/server/models/room.js +++ b/server/models/room.js @@ -211,9 +211,7 @@ Room.methods.dropUser = async function (user, updateAdmin) { this.inRoom.set(user.id.toString(), false); this.waitingFor.set(user.id.toString(), false); - if (updateAdmin) { - await this.updateAdminIfNeeded(updateAdmin); - } + await this.updateAdminIfNeeded(updateAdmin); if (this.usersInRoom.length === 0 && this.type === 'normal') { await this.updateStale(true); @@ -340,7 +338,9 @@ Room.methods.updateAdminIfNeeded = async function (cb) { this.admin = nextAdmin; const room = await this.save(); - if (cb) { + // UPDATE_ADMIN has always carried a user. Empty rooms persist null without + // notifying clients that still expect a populated admin object. + if (cb && room.admin) { cb(room); } return room; diff --git a/server/models/room.test.js b/server/models/room.test.js index 752a519..8c1dc35 100644 --- a/server/models/room.test.js +++ b/server/models/room.test.js @@ -210,6 +210,56 @@ describe('room security helpers', () => { })).toBeNull(); }); + it('hands off and persists admin after a removal without a callback', async () => { + const owner = { id: 101 }; + const nextAdmin = { id: 202 }; + const persistedAdmins = []; + const room = { + owner, + admin: owner, + inRoom: new Map([['101', true], ['202', true]]), + waitingFor: new Map([['101', true], ['202', true]]), + type: 'grand_prix', + updateAdminIfNeeded: Room.methods.updateAdminIfNeeded, + save: jest.fn(async () => { + persistedAdmins.push(room.admin && room.admin.id); + return room; + }), + }; + Object.defineProperty(room, 'usersInRoom', { + get: () => [owner, nextAdmin].filter((user) => room.inRoom.get(String(user.id))), + }); + + await Room.methods.dropUser.call(room, owner); + + expect(room.admin).toBe(nextAdmin); + expect(persistedAdmins).toContain(nextAdmin.id); + }); + + it('persists an empty room without announcing a null admin', async () => { + const owner = { id: 101 }; + const onAdminChange = jest.fn(); + const room = { + owner, + admin: owner, + inRoom: new Map([['101', true]]), + waitingFor: new Map([['101', true]]), + type: 'grand_prix', + updateAdminIfNeeded: Room.methods.updateAdminIfNeeded, + save: jest.fn().mockResolvedValue(undefined), + }; + Object.defineProperty(room, 'usersInRoom', { + get: () => (room.inRoom.get('101') ? [owner] : []), + }); + room.save.mockResolvedValue(room); + + await Room.methods.dropUser.call(room, owner, onAdminChange); + + expect(room.admin).toBeNull(); + expect(room.save).toHaveBeenCalled(); + expect(onAdminChange).not.toHaveBeenCalled(); + }); + it('persists and announces the owner reclaiming admin controls', async () => { const owner = { id: 101 }; const room = { diff --git a/server/socket/namespaces/rooms.js b/server/socket/namespaces/rooms.js index 2e0cc9c..ed6e14e 100644 --- a/server/socket/namespaces/rooms.js +++ b/server/socket/namespaces/rooms.js @@ -118,6 +118,12 @@ module.exports = (io, middlewares) => { } } + function sendAdminUpdate(room) { + if (room.admin) { + ns().in(room.accessCode).emit(Protocol.UPDATE_ADMIN, room.admin); + } + } + async function finalizeUserDeparture({ roomId, userId, connectionId, leaveReason, }) { @@ -133,9 +139,7 @@ module.exports = (io, middlewares) => { return false; } - const updatedRoom = await room.dropUser({ id: userId }, (_room) => { - ns().in(room.accessCode).emit(Protocol.UPDATE_ADMIN, _room.admin); - }); + const updatedRoom = await room.dropUser({ id: userId }, sendAdminUpdate); await metrics.endRoomVisit({ room: updatedRoom, @@ -406,9 +410,7 @@ module.exports = (io, middlewares) => { }); } - const r = await activeRoom.addUser(socket.user, spectating, (_room) => { - ns().in(_room.accessCode).emit(Protocol.UPDATE_ADMIN, _room.admin); - }); + const r = await activeRoom.addUser(socket.user, spectating, sendAdminUpdate); if (!r) { // The user is already present in persisted room state. @@ -920,6 +922,15 @@ module.exports = (io, middlewares) => { return; } + if (String(userId) === String(socket.user.id)) { + socket.emit(Protocol.ERROR, { + statusCode: 400, + event: Protocol.KICK_USER, + message: 'Cannot kick yourself; leave the room instead', + }); + return; + } + try { const room = await socket.room.dropUser({ id: userId }); @@ -954,6 +965,15 @@ module.exports = (io, middlewares) => { return; } + if (String(userId) === String(socket.user.id)) { + socket.emit(Protocol.ERROR, { + statusCode: 400, + event: Protocol.BAN_USER, + message: 'Cannot ban yourself; leave the room instead', + }); + return; + } + try { const room = await socket.room.banUser(userId); @@ -1058,8 +1078,11 @@ module.exports = (io, middlewares) => { on(Protocol.LEAVE_ROOM, async () => { if (socket.room) { if (socket.user) { - await leaveRoom('explicit'); + // Exclude this tab before checking whether another tab keeps the + // user's room membership active. Otherwise simultaneous leaves can + // each see the other socket and neither finalizes the departure. socket.leave(encodeUserRoom(socket.userId, socket.room._id)); + await leaveRoom('explicit'); } else { await metrics.endRoomVisit({ room: socket.room, diff --git a/server/socket/namespaces/rooms.test.js b/server/socket/namespaces/rooms.test.js index b153e5d..b6fbf2c 100644 --- a/server/socket/namespaces/rooms.test.js +++ b/server/socket/namespaces/rooms.test.js @@ -3,6 +3,8 @@ const Protocol = require('../../../client/src/lib/protocol.json'); const metrics = require('../../metrics'); const { Room, User } = require('../../models'); +const { encodeUserRoom } = require('../utils'); +const { createReconnectGrace } = require('../lib/reconnectGrace'); const initRooms = require('./rooms'); jest.mock('../../runtimeConfig', () => ({ @@ -33,9 +35,10 @@ jest.mock('../../postgres/dualWrite', () => ({ markRoomDeleted: jest.fn(), })); jest.mock('../lib/reconnectGrace', () => ({ - createReconnectGrace: jest.fn(() => ({ + createReconnectGrace: jest.fn(({ finalizeDeparture }) => ({ cancel: jest.fn().mockResolvedValue(false), finalize: jest.fn(), + finalizeDeparture, schedule: jest.fn(), startReconciliation: jest.fn(), })), @@ -345,3 +348,106 @@ describe('room namespace edits', () => { })); }); }); + +describe('room namespace departures and moderation', () => { + beforeEach(() => { + jest.clearAllMocks(); + metrics.endRoomVisit.mockResolvedValue(undefined); + }); + + it('does not send a null admin update to a spectator channel when a room empties', async () => { + const room = makeRoom({ + private: false, + password: null, + usersLength: 0, + inRoom: new Map([['101', true]]), + doneWithScramble: jest.fn().mockReturnValue(false), + }); + room.dropUser = jest.fn(async (user, onAdminChange) => { + room.inRoom.set(String(user.id), false); + room.admin = null; + onAdminChange(room); + return room; + }); + const connect = setup(new Map([[room._id, room]])); + await connect(makeSocket({ id: 'spectator-socket', session: {} })); + const reconnectGrace = createReconnectGrace.mock.results[0].value; + + await reconnectGrace.finalizeDeparture({ + roomId: room._id, + userId: 101, + connectionId: 'host-socket', + leaveReason: 'explicit', + }); + + expect(connect.roomChannel.emit).not.toHaveBeenCalledWith(Protocol.UPDATE_ADMIN, null); + expect(connect.roomChannel.emit).toHaveBeenCalledWith(Protocol.USER_LEFT, 101); + }); + + it('finalizes one departure when two tabs explicitly leave together', async () => { + const room = makeRoom({ + private: false, + password: null, + inRoom: new Map([['101', true]]), + }); + const connect = setup(new Map([[room._id, room]])); + const first = makeSocket({ id: 'first-tab', session: {}, userId: 101 }); + const second = makeSocket({ id: 'second-tab', session: {}, userId: 101 }); + const activeSocketIds = new Set([first.id, second.id]); + const userRoom = encodeUserRoom(101, room._id); + connect.namespace.adapter.sockets.mockImplementation(async () => new Set(activeSocketIds)); + [first, second].forEach((socket) => { + socket.room = room; + socket.roomId = room._id; + socket.leave.mockImplementation((roomName) => { + if (roomName === userRoom) { + activeSocketIds.delete(socket.id); + } + }); + }); + await connect(first); + await connect(second); + const reconnectGrace = createReconnectGrace.mock.results[0].value; + + await Promise.all([ + first.handlers[Protocol.LEAVE_ROOM](), + second.handlers[Protocol.LEAVE_ROOM](), + ]); + + expect(reconnectGrace.finalize).toHaveBeenCalledTimes(1); + expect(reconnectGrace.finalize).toHaveBeenCalledWith(expect.objectContaining({ + roomId: room._id, + userId: 101, + leaveReason: 'explicit', + })); + }); + + it('rejects raw self-kick and self-ban requests from an admin', async () => { + const room = makeRoom({ + private: false, + password: null, + admin: { id: 101 }, + dropUser: jest.fn(), + banUser: jest.fn(), + }); + const connect = setup(new Map([[room._id, room]])); + const socket = makeSocket({ id: 'admin-socket', session: {}, userId: 101 }); + socket.room = room; + socket.roomId = room._id; + await connect(socket); + + await socket.handlers[Protocol.KICK_USER](101); + await socket.handlers[Protocol.BAN_USER]('101'); + + expect(room.dropUser).not.toHaveBeenCalled(); + expect(room.banUser).not.toHaveBeenCalled(); + expect(socket.emit).toHaveBeenCalledWith(Protocol.ERROR, expect.objectContaining({ + statusCode: 400, + event: Protocol.KICK_USER, + })); + expect(socket.emit).toHaveBeenCalledWith(Protocol.ERROR, expect.objectContaining({ + statusCode: 400, + event: Protocol.BAN_USER, + })); + }); +}); From 9e572b45df3fcd4f547a453ed2ce4c1b7f2af8ef Mon Sep 17 00:00:00 2001 From: Cailyn Sinclair Date: Sun, 12 Jul 2026 21:34:22 -0700 Subject: [PATCH 3/6] Make room departures cross-process safe Claim persisted room departures with a MongoDB compare-and-set on the active membership revision. This prevents competing Socket.IO processes from duplicating leave side effects or overwriting a rejoin, while retaining the reconnect grace behavior.\n\nRefs #153 --- docs/room-ownership.md | 6 ++ server/models/room.js | 104 +++++++++++++++++++++---- server/models/room.test.js | 52 +++++++++++++ server/socket/namespaces/rooms.js | 31 ++++++-- server/socket/namespaces/rooms.test.js | 60 ++++++++++++-- 5 files changed, 226 insertions(+), 27 deletions(-) diff --git a/docs/room-ownership.md b/docs/room-ownership.md index 48e5131..987a8ad 100644 --- a/docs/room-ownership.md +++ b/docs/room-ownership.md @@ -34,6 +34,12 @@ itself from the per-user socket group before the remaining-tab check, so two simultaneous explicit leaves cannot strand membership. If the owner later rejoins, they reclaim admin before the join is acknowledged. +The final persisted departure is a MongoDB compare-and-set keyed by the active +membership revision. This applies across Socket.IO processes: exactly one +process can claim a leave, emit its room events, and record its metrics. A +concurrent rejoin advances the revision, so a losing leave retries from current +state and cannot overwrite the new membership. + An empty room persists `admin: null`, but the established `UPDATE_ADMIN` Socket.IO event is emitted only for a non-null active admin. Spectators and other clients therefore never receive a null admin payload. diff --git a/server/models/room.js b/server/models/room.js index bc4bdc3..3c761ff 100644 --- a/server/models/room.js +++ b/server/models/room.js @@ -7,6 +7,22 @@ const { mirrorRoomChanges } = require('../postgres/dualWrite'); const PASSWORD_SALT_ROUNDS = 10; const STALE_ROOM_LIFETIME_MS = 10 * 60 * 1000; +const sameUser = (left, right) => left && right + && String(left.id) === String(right.id); + +const selectRoomAdmin = ({ usersInRoom, owner, admin }) => { + if (usersInRoom.length === 0) { + return null; + } + + const activeOwner = usersInRoom.find((user) => sameUser(user, owner)); + if (activeOwner) { + return activeOwner; + } + + return usersInRoom.find((user) => sameUser(user, admin)) || usersInRoom[0]; +}; + // const lengths = { // }; @@ -87,6 +103,12 @@ const Room = new mongoose.Schema({ of: Boolean, default: {}, }, + // Incremented for every join or departure so a conditional departure cannot + // overwrite a reconnect that raced it on another Socket.IO process. + membershipRevision: { + type: Number, + default: 0, + }, admin: { type: mongoose.Schema.Types.ObjectId, ref: 'User' }, owner: { type: mongoose.Schema.Types.ObjectId, ref: 'User' }, type: { @@ -187,6 +209,7 @@ Room.methods.addUser = async function (user, spectating, updateAdmin) { } this.inRoom.set(user.id.toString(), true); + this.membershipRevision = (this.membershipRevision || 0) + 1; if (this.waitingForCount === 0) { this.waitingFor.set(user.id.toString(), true); @@ -210,6 +233,7 @@ Room.methods.addUser = async function (user, spectating, updateAdmin) { Room.methods.dropUser = async function (user, updateAdmin) { this.inRoom.set(user.id.toString(), false); this.waitingFor.set(user.id.toString(), false); + this.membershipRevision = (this.membershipRevision || 0) + 1; await this.updateAdminIfNeeded(updateAdmin); @@ -220,6 +244,70 @@ Room.methods.dropUser = async function (user, updateAdmin) { return this.save(); }; +Room.methods.dropUserAtomically = async function (user) { + const userKey = user.id.toString(); + if (!this.inRoom.get(userKey)) { + return null; + } + + const usersInRoom = this.users.filter((candidate) => ( + candidate.id.toString() !== userKey + && this.inRoom.get(candidate.id.toString()) + )); + const nextAdmin = selectRoomAdmin({ + usersInRoom, + owner: this.owner, + admin: this.admin, + }); + const adminChanged = !sameUser(this.admin, nextAdmin); + const membershipRevision = this.membershipRevision || 0; + const condition = { + _id: this._id, + [`inRoom.${userKey}`]: true, + }; + if (this.updatedAt) { + condition.updatedAt = this.updatedAt; + } + if (membershipRevision > 0) { + condition.membershipRevision = membershipRevision; + } else { + condition.$or = [ + { membershipRevision: 0 }, + { membershipRevision: { $exists: false } }, + ]; + } + + const update = { + $set: { + [`inRoom.${userKey}`]: false, + [`waitingFor.${userKey}`]: false, + admin: nextAdmin ? nextAdmin._id || nextAdmin : null, + }, + $inc: { + membershipRevision: 1, + }, + }; + if (usersInRoom.length === 0 && this.type === 'normal') { + update.$set.expireAt = new Date(Date.now() + STALE_ROOM_LIFETIME_MS); + } + + const updatedRoom = await this.constructor.findOneAndUpdate(condition, update, { + new: true, + }).populate('users').populate('admin').populate('owner'); + if (!updatedRoom) { + return null; + } + + await mirrorRoomChanges(updatedRoom, { + attempts: [], + participantUserIds: [userKey], + syncAllParticipants: false, + syncRoomOwners: adminChanged, + }); + + return { room: updatedRoom, adminChanged }; +}; + Room.methods.banUser = async function (userId) { this.banned.set(userId.toString(), true); return this.dropUser({ id: userId }); @@ -314,22 +402,6 @@ Room.methods.edit = async function (options) { return this.save(); }; -const sameUser = (left, right) => left && right - && String(left.id) === String(right.id); - -const selectRoomAdmin = ({ usersInRoom, owner, admin }) => { - if (usersInRoom.length === 0) { - return null; - } - - const activeOwner = usersInRoom.find((user) => sameUser(user, owner)); - if (activeOwner) { - return activeOwner; - } - - return usersInRoom.find((user) => sameUser(user, admin)) || usersInRoom[0]; -}; - Room.methods.updateAdminIfNeeded = async function (cb) { const nextAdmin = selectRoomAdmin(this); if (sameUser(this.admin, nextAdmin) || (!this.admin && !nextAdmin)) { diff --git a/server/models/room.test.js b/server/models/room.test.js index 8c1dc35..44a6e71 100644 --- a/server/models/room.test.js +++ b/server/models/room.test.js @@ -4,6 +4,7 @@ const bcrypt = require('bcrypt'); const mongoose = require('mongoose'); const { generateScramble } = require('letscube-scrambles'); +const { mirrorRoomChanges } = require('../postgres/dualWrite'); const { collectPostgresChanges, Room, @@ -13,6 +14,9 @@ const { jest.mock('letscube-scrambles', () => ({ generateScramble: jest.fn(), })); +jest.mock('../postgres/dualWrite', () => ({ + mirrorRoomChanges: jest.fn().mockResolvedValue(undefined), +})); const RoomModel = mongoose.model('RoomPostgresChangesTest', Room); @@ -260,6 +264,54 @@ describe('room security helpers', () => { expect(onAdminChange).not.toHaveBeenCalled(); }); + it('claims a departure with the membership revision compare-and-set', async () => { + const owner = { id: 101, _id: 'owner-id' }; + const nextAdmin = { id: 202, _id: 'next-admin-id' }; + const updatedRoom = { id: 'room-one' }; + const query = { + populate: jest.fn(() => query), + then: (resolve, reject) => Promise.resolve(updatedRoom).then(resolve, reject), + }; + const model = { + findOneAndUpdate: jest.fn(() => query), + }; + const updatedAt = new Date('2026-07-13T04:30:00.000Z'); + const room = { + _id: 'room-one', + owner, + admin: owner, + users: [owner, nextAdmin], + inRoom: new Map([['101', true], ['202', true]]), + type: 'normal', + membershipRevision: 7, + updatedAt, + constructor: model, + }; + + const departure = await Room.methods.dropUserAtomically.call(room, owner); + + expect(model.findOneAndUpdate).toHaveBeenCalledWith({ + _id: room._id, + 'inRoom.101': true, + membershipRevision: 7, + updatedAt, + }, { + $set: { + 'inRoom.101': false, + 'waitingFor.101': false, + admin: nextAdmin._id, + }, + $inc: { membershipRevision: 1 }, + }, { new: true }); + expect(departure).toEqual({ room: updatedRoom, adminChanged: true }); + expect(mirrorRoomChanges).toHaveBeenCalledWith(updatedRoom, { + attempts: [], + participantUserIds: ['101'], + syncAllParticipants: false, + syncRoomOwners: true, + }); + }); + it('persists and announces the owner reclaiming admin controls', async () => { const owner = { id: 101 }; const room = { diff --git a/server/socket/namespaces/rooms.js b/server/socket/namespaces/rooms.js index ed6e14e..1249c9e 100644 --- a/server/socket/namespaces/rooms.js +++ b/server/socket/namespaces/rooms.js @@ -127,19 +127,38 @@ module.exports = (io, middlewares) => { async function finalizeUserDeparture({ roomId, userId, connectionId, leaveReason, }) { - const room = await fetchRoom(roomId); const userKey = userId.toString(); - if (!room || !room.inRoom.get(userKey)) { - return false; + // The compare-and-set in dropUserAtomically is the cross-process claim. + // A losing process re-reads so it never overwrites a concurrent rejoin. + async function claimDeparture() { + const room = await fetchRoom(roomId); + if (!room || !room.inRoom.get(userKey)) { + return null; + } + + if (leaveReason === 'disconnect' + && await hasActiveSocketsForUserRoom(userId, roomId)) { + return null; + } + + const departure = await room.dropUserAtomically({ id: userId }); + if (!departure) { + return claimDeparture(); + } + + return departure; } - if (leaveReason === 'disconnect' - && await hasActiveSocketsForUserRoom(userId, roomId)) { + const departure = await claimDeparture(); + if (!departure) { return false; } - const updatedRoom = await room.dropUser({ id: userId }, sendAdminUpdate); + const { room: updatedRoom, adminChanged } = departure; + if (adminChanged) { + sendAdminUpdate(updatedRoom); + } await metrics.endRoomVisit({ room: updatedRoom, diff --git a/server/socket/namespaces/rooms.test.js b/server/socket/namespaces/rooms.test.js index b6fbf2c..04fa4af 100644 --- a/server/socket/namespaces/rooms.test.js +++ b/server/socket/namespaces/rooms.test.js @@ -363,11 +363,12 @@ describe('room namespace departures and moderation', () => { inRoom: new Map([['101', true]]), doneWithScramble: jest.fn().mockReturnValue(false), }); - room.dropUser = jest.fn(async (user, onAdminChange) => { - room.inRoom.set(String(user.id), false); - room.admin = null; - onAdminChange(room); - return room; + room.dropUserAtomically = jest.fn().mockResolvedValue({ + room: { + ...room, + admin: null, + }, + adminChanged: true, }); const connect = setup(new Map([[room._id, room]])); await connect(makeSocket({ id: 'spectator-socket', session: {} })); @@ -384,6 +385,55 @@ describe('room namespace departures and moderation', () => { expect(connect.roomChannel.emit).toHaveBeenCalledWith(Protocol.USER_LEFT, 101); }); + it('emits metrics and leave events only for the process that claims a departure', async () => { + const room = makeRoom({ + private: false, + password: null, + inRoom: new Map([['101', true]]), + }); + const updatedRoom = { + ...room, + inRoom: new Map([['101', false]]), + usersLength: 0, + doneWithScramble: jest.fn().mockReturnValue(false), + }; + let claimed = false; + room.dropUserAtomically = jest.fn(async () => { + if (claimed) { + return null; + } + claimed = true; + return { room: updatedRoom, adminChanged: false }; + }); + const connect = setup(new Map([[room._id, room]])); + const persistedRoom = { + ...room, + inRoom: new Map([['101', false]]), + }; + Room.findById.mockImplementation(() => queryResult(claimed ? persistedRoom : room)); + const reconnectGrace = createReconnectGrace.mock.results[0].value; + + await Promise.all([ + reconnectGrace.finalizeDeparture({ + roomId: room._id, + userId: 101, + connectionId: 'socket-on-process-a', + leaveReason: 'explicit', + }), + reconnectGrace.finalizeDeparture({ + roomId: room._id, + userId: 101, + connectionId: 'socket-on-process-b', + leaveReason: 'explicit', + }), + ]); + + expect(room.dropUserAtomically).toHaveBeenCalledTimes(2); + expect(metrics.endRoomVisit).toHaveBeenCalledTimes(1); + expect(connect.roomChannel.emit).toHaveBeenCalledTimes(1); + expect(connect.roomChannel.emit).toHaveBeenCalledWith(Protocol.USER_LEFT, 101); + }); + it('finalizes one departure when two tabs explicitly leave together', async () => { const room = makeRoom({ private: false, From 7f9998703ad05e7898fa8e020b9a3a731683850b Mon Sep 17 00:00:00 2001 From: Cailyn Sinclair Date: Sun, 12 Jul 2026 21:39:49 -0700 Subject: [PATCH 4/6] Make stale departure claims terminal Carry the membership revision captured when a socket leaves through reconnect cleanup and reject a departure when that generation no longer matches. This prevents a losing old finalizer from removing a later rejoin.\n\nRefs #153 --- docs/room-ownership.md | 4 +- server/models/room.js | 12 ++--- server/models/room.test.js | 3 -- server/socket/namespaces/rooms.js | 35 ++++++-------- server/socket/namespaces/rooms.test.js | 63 +++++++++++++++----------- 5 files changed, 59 insertions(+), 58 deletions(-) diff --git a/docs/room-ownership.md b/docs/room-ownership.md index 987a8ad..dd4a3f6 100644 --- a/docs/room-ownership.md +++ b/docs/room-ownership.md @@ -37,8 +37,8 @@ rejoins, they reclaim admin before the join is acknowledged. The final persisted departure is a MongoDB compare-and-set keyed by the active membership revision. This applies across Socket.IO processes: exactly one process can claim a leave, emit its room events, and record its metrics. A -concurrent rejoin advances the revision, so a losing leave retries from current -state and cannot overwrite the new membership. +concurrent rejoin advances the revision, so a losing leave is terminal and +cannot overwrite the new membership. An empty room persists `admin: null`, but the established `UPDATE_ADMIN` Socket.IO event is emitted only for a non-null active admin. Spectators and diff --git a/server/models/room.js b/server/models/room.js index 3c761ff..6b0c22a 100644 --- a/server/models/room.js +++ b/server/models/room.js @@ -244,9 +244,13 @@ Room.methods.dropUser = async function (user, updateAdmin) { return this.save(); }; -Room.methods.dropUserAtomically = async function (user) { +Room.methods.dropUserAtomically = async function ( + user, + expectedMembershipRevision = this.membershipRevision || 0, +) { const userKey = user.id.toString(); - if (!this.inRoom.get(userKey)) { + const membershipRevision = this.membershipRevision || 0; + if (!this.inRoom.get(userKey) || expectedMembershipRevision !== membershipRevision) { return null; } @@ -260,14 +264,10 @@ Room.methods.dropUserAtomically = async function (user) { admin: this.admin, }); const adminChanged = !sameUser(this.admin, nextAdmin); - const membershipRevision = this.membershipRevision || 0; const condition = { _id: this._id, [`inRoom.${userKey}`]: true, }; - if (this.updatedAt) { - condition.updatedAt = this.updatedAt; - } if (membershipRevision > 0) { condition.membershipRevision = membershipRevision; } else { diff --git a/server/models/room.test.js b/server/models/room.test.js index 44a6e71..35d0bc8 100644 --- a/server/models/room.test.js +++ b/server/models/room.test.js @@ -275,7 +275,6 @@ describe('room security helpers', () => { const model = { findOneAndUpdate: jest.fn(() => query), }; - const updatedAt = new Date('2026-07-13T04:30:00.000Z'); const room = { _id: 'room-one', owner, @@ -284,7 +283,6 @@ describe('room security helpers', () => { inRoom: new Map([['101', true], ['202', true]]), type: 'normal', membershipRevision: 7, - updatedAt, constructor: model, }; @@ -294,7 +292,6 @@ describe('room security helpers', () => { _id: room._id, 'inRoom.101': true, membershipRevision: 7, - updatedAt, }, { $set: { 'inRoom.101': false, diff --git a/server/socket/namespaces/rooms.js b/server/socket/namespaces/rooms.js index 1249c9e..036eac6 100644 --- a/server/socket/namespaces/rooms.js +++ b/server/socket/namespaces/rooms.js @@ -87,6 +87,7 @@ module.exports = (io, middlewares) => { return rooms.flatMap((room) => room.usersInRoom.map((user) => ({ roomId: room._id, userId: user.id, + membershipRevision: room.membershipRevision, leaveReason: 'disconnect', }))); } @@ -125,32 +126,24 @@ module.exports = (io, middlewares) => { } async function finalizeUserDeparture({ - roomId, userId, connectionId, leaveReason, + roomId, userId, connectionId, leaveReason, membershipRevision, }) { const userKey = userId.toString(); - // The compare-and-set in dropUserAtomically is the cross-process claim. - // A losing process re-reads so it never overwrites a concurrent rejoin. - async function claimDeparture() { - const room = await fetchRoom(roomId); - if (!room || !room.inRoom.get(userKey)) { - return null; - } - - if (leaveReason === 'disconnect' - && await hasActiveSocketsForUserRoom(userId, roomId)) { - return null; - } - - const departure = await room.dropUserAtomically({ id: userId }); - if (!departure) { - return claimDeparture(); - } + const room = await fetchRoom(roomId); + if (!room || !room.inRoom.get(userKey) + || room.membershipRevision !== membershipRevision) { + return false; + } - return departure; + if (leaveReason === 'disconnect' + && await hasActiveSocketsForUserRoom(userId, roomId)) { + return false; } - const departure = await claimDeparture(); + // The membership revision is the departure generation. A failed claim is + // terminal: retrying it after a rejoin would remove the new membership. + const departure = await room.dropUserAtomically({ id: userId }, membershipRevision); if (!departure) { return false; } @@ -378,6 +371,7 @@ module.exports = (io, middlewares) => { roomId: socket.roomId, userId: socket.userId, connectionId: socket.id, + membershipRevision: socket.room.membershipRevision, leaveReason, }); } catch (e) { @@ -1077,6 +1071,7 @@ module.exports = (io, middlewares) => { roomId: socket.roomId, userId: socket.userId, connectionId: socket.id, + membershipRevision: socket.room.membershipRevision, leaveReason: 'disconnect', }); } else if (socket.room) { diff --git a/server/socket/namespaces/rooms.test.js b/server/socket/namespaces/rooms.test.js index 04fa4af..16148f0 100644 --- a/server/socket/namespaces/rooms.test.js +++ b/server/socket/namespaces/rooms.test.js @@ -71,6 +71,7 @@ const makeRoom = (overrides = {}) => ({ inRoom: new Map(), banned: new Map(), registered: new Map(), + membershipRevision: 7, requireRevealedIdentity: false, usersLength: 1, authenticate: jest.fn().mockResolvedValue(true), @@ -378,6 +379,7 @@ describe('room namespace departures and moderation', () => { roomId: room._id, userId: 101, connectionId: 'host-socket', + membershipRevision: room.membershipRevision, leaveReason: 'explicit', }); @@ -385,7 +387,7 @@ describe('room namespace departures and moderation', () => { expect(connect.roomChannel.emit).toHaveBeenCalledWith(Protocol.USER_LEFT, 101); }); - it('emits metrics and leave events only for the process that claims a departure', async () => { + it('does not let a losing departure remove a later rejoin', async () => { const room = makeRoom({ private: false, password: null, @@ -394,41 +396,47 @@ describe('room namespace departures and moderation', () => { const updatedRoom = { ...room, inRoom: new Map([['101', false]]), + membershipRevision: 8, usersLength: 0, doneWithScramble: jest.fn().mockReturnValue(false), }; - let claimed = false; - room.dropUserAtomically = jest.fn(async () => { - if (claimed) { - return null; - } - claimed = true; - return { room: updatedRoom, adminChanged: false }; + room.dropUserAtomically = jest.fn().mockResolvedValue({ + room: updatedRoom, + adminChanged: false, }); const connect = setup(new Map([[room._id, room]])); - const persistedRoom = { + let persistedRoom = room; + Room.findById.mockImplementation(() => queryResult(persistedRoom)); + const reconnectGrace = createReconnectGrace.mock.results[0].value; + + await expect(reconnectGrace.finalizeDeparture({ + roomId: room._id, + userId: 101, + connectionId: 'winner-socket', + membershipRevision: 7, + leaveReason: 'explicit', + })).resolves.toBe(true); + + const rejoinedRoom = { ...room, - inRoom: new Map([['101', false]]), + admin: { id: 101 }, + inRoom: new Map([['101', true]]), + membershipRevision: 9, }; - Room.findById.mockImplementation(() => queryResult(claimed ? persistedRoom : room)); - const reconnectGrace = createReconnectGrace.mock.results[0].value; + persistedRoom = rejoinedRoom; - await Promise.all([ - reconnectGrace.finalizeDeparture({ - roomId: room._id, - userId: 101, - connectionId: 'socket-on-process-a', - leaveReason: 'explicit', - }), - reconnectGrace.finalizeDeparture({ - roomId: room._id, - userId: 101, - connectionId: 'socket-on-process-b', - leaveReason: 'explicit', - }), - ]); + await expect(reconnectGrace.finalizeDeparture({ + roomId: room._id, + userId: 101, + connectionId: 'loser-socket', + membershipRevision: 7, + leaveReason: 'explicit', + })).resolves.toBe(false); - expect(room.dropUserAtomically).toHaveBeenCalledTimes(2); + expect(room.dropUserAtomically).toHaveBeenCalledTimes(1); + expect(rejoinedRoom.inRoom.get('101')).toBe(true); + expect(rejoinedRoom.admin).toEqual({ id: 101 }); + expect(rejoinedRoom.membershipRevision).toBe(updatedRoom.membershipRevision + 1); expect(metrics.endRoomVisit).toHaveBeenCalledTimes(1); expect(connect.roomChannel.emit).toHaveBeenCalledTimes(1); expect(connect.roomChannel.emit).toHaveBeenCalledWith(Protocol.USER_LEFT, 101); @@ -468,6 +476,7 @@ describe('room namespace departures and moderation', () => { expect(reconnectGrace.finalize).toHaveBeenCalledWith(expect.objectContaining({ roomId: room._id, userId: 101, + membershipRevision: room.membershipRevision, leaveReason: 'explicit', })); }); From af3602b8ff433335cb02d11037931728033ae37c Mon Sep 17 00:00:00 2001 From: Cailyn Sinclair Date: Sun, 12 Jul 2026 21:48:12 -0700 Subject: [PATCH 5/6] Fence duplicate room joins from stale leaves Advance a durable per-user presence revision before acknowledging an already-active room membership. Departure claims now require that generation, so an old leave cannot remove a newly opened tab.\n\nRefs #153 --- docs/room-ownership.md | 11 +- server/models/room.js | 63 +++++++++++- server/models/room.test.js | 31 +++++- server/socket/namespaces/rooms.js | 50 ++++++++-- server/socket/namespaces/rooms.test.js | 133 +++++++++++++++++++++---- 5 files changed, 248 insertions(+), 40 deletions(-) diff --git a/docs/room-ownership.md b/docs/room-ownership.md index dd4a3f6..3b1297e 100644 --- a/docs/room-ownership.md +++ b/docs/room-ownership.md @@ -34,11 +34,12 @@ itself from the per-user socket group before the remaining-tab check, so two simultaneous explicit leaves cannot strand membership. If the owner later rejoins, they reclaim admin before the join is acknowledged. -The final persisted departure is a MongoDB compare-and-set keyed by the active -membership revision. This applies across Socket.IO processes: exactly one -process can claim a leave, emit its room events, and record its metrics. A -concurrent rejoin advances the revision, so a losing leave is terminal and -cannot overwrite the new membership. +The final persisted departure is a MongoDB compare-and-set keyed by room +membership and the active user's presence revision. This applies across +Socket.IO processes: exactly one process can claim a leave, emit its room +events, and record its metrics. Every authenticated duplicate join advances +that user's durable presence revision before acknowledgment, so an older leave +is terminal and cannot overwrite an active new tab. An empty room persists `admin: null`, but the established `UPDATE_ADMIN` Socket.IO event is emitted only for a non-null active admin. Spectators and diff --git a/server/models/room.js b/server/models/room.js index 6b0c22a..275f102 100644 --- a/server/models/room.js +++ b/server/models/room.js @@ -23,6 +23,17 @@ const selectRoomAdmin = ({ usersInRoom, owner, admin }) => { return usersInRoom.find((user) => sameUser(user, admin)) || usersInRoom[0]; }; +const presenceRevisionFor = (room, userKey) => ( + room.presenceRevision ? room.presenceRevision.get(userKey) || 0 : 0 +); + +const advancePresenceRevisionFor = (room, userKey) => { + if (!room.presenceRevision) { + room.presenceRevision = new Map(); + } + room.presenceRevision.set(userKey, presenceRevisionFor(room, userKey) + 1); +}; + // const lengths = { // }; @@ -109,6 +120,11 @@ const Room = new mongoose.Schema({ type: Number, default: 0, }, + presenceRevision: { + type: Map, + of: Number, + default: {}, + }, admin: { type: mongoose.Schema.Types.ObjectId, ref: 'User' }, owner: { type: mongoose.Schema.Types.ObjectId, ref: 'User' }, type: { @@ -210,6 +226,7 @@ Room.methods.addUser = async function (user, spectating, updateAdmin) { this.inRoom.set(user.id.toString(), true); this.membershipRevision = (this.membershipRevision || 0) + 1; + advancePresenceRevisionFor(this, user.id.toString()); if (this.waitingForCount === 0) { this.waitingFor.set(user.id.toString(), true); @@ -234,6 +251,7 @@ Room.methods.dropUser = async function (user, updateAdmin) { this.inRoom.set(user.id.toString(), false); this.waitingFor.set(user.id.toString(), false); this.membershipRevision = (this.membershipRevision || 0) + 1; + advancePresenceRevisionFor(this, user.id.toString()); await this.updateAdminIfNeeded(updateAdmin); @@ -247,10 +265,14 @@ Room.methods.dropUser = async function (user, updateAdmin) { Room.methods.dropUserAtomically = async function ( user, expectedMembershipRevision = this.membershipRevision || 0, + expectedPresenceRevision = presenceRevisionFor(this, user.id.toString()), ) { const userKey = user.id.toString(); const membershipRevision = this.membershipRevision || 0; - if (!this.inRoom.get(userKey) || expectedMembershipRevision !== membershipRevision) { + const presenceRevision = presenceRevisionFor(this, userKey); + if (!this.inRoom.get(userKey) + || expectedMembershipRevision !== membershipRevision + || expectedPresenceRevision !== presenceRevision) { return null; } @@ -268,13 +290,29 @@ Room.methods.dropUserAtomically = async function ( _id: this._id, [`inRoom.${userKey}`]: true, }; + const revisionConditions = []; if (membershipRevision > 0) { condition.membershipRevision = membershipRevision; } else { - condition.$or = [ - { membershipRevision: 0 }, - { membershipRevision: { $exists: false } }, - ]; + revisionConditions.push({ + $or: [ + { membershipRevision: 0 }, + { membershipRevision: { $exists: false } }, + ], + }); + } + if (presenceRevision > 0) { + condition[`presenceRevision.${userKey}`] = presenceRevision; + } else { + revisionConditions.push({ + $or: [ + { [`presenceRevision.${userKey}`]: 0 }, + { [`presenceRevision.${userKey}`]: { $exists: false } }, + ], + }); + } + if (revisionConditions.length > 0) { + condition.$and = revisionConditions; } const update = { @@ -285,6 +323,7 @@ Room.methods.dropUserAtomically = async function ( }, $inc: { membershipRevision: 1, + [`presenceRevision.${userKey}`]: 1, }, }; if (usersInRoom.length === 0 && this.type === 'normal') { @@ -308,6 +347,20 @@ Room.methods.dropUserAtomically = async function ( return { room: updatedRoom, adminChanged }; }; +Room.methods.advancePresenceRevision = function (userId) { + const userKey = userId.toString(); + return this.constructor.findOneAndUpdate({ + _id: this._id, + [`inRoom.${userKey}`]: true, + }, { + $inc: { + [`presenceRevision.${userKey}`]: 1, + }, + }, { + new: true, + }).populate('users').populate('admin').populate('owner'); +}; + Room.methods.banUser = async function (userId) { this.banned.set(userId.toString(), true); return this.dropUser({ id: userId }); diff --git a/server/models/room.test.js b/server/models/room.test.js index 35d0bc8..ea83799 100644 --- a/server/models/room.test.js +++ b/server/models/room.test.js @@ -283,6 +283,7 @@ describe('room security helpers', () => { inRoom: new Map([['101', true], ['202', true]]), type: 'normal', membershipRevision: 7, + presenceRevision: new Map([['101', 7]]), constructor: model, }; @@ -292,13 +293,17 @@ describe('room security helpers', () => { _id: room._id, 'inRoom.101': true, membershipRevision: 7, + 'presenceRevision.101': 7, }, { $set: { 'inRoom.101': false, 'waitingFor.101': false, admin: nextAdmin._id, }, - $inc: { membershipRevision: 1 }, + $inc: { + membershipRevision: 1, + 'presenceRevision.101': 1, + }, }, { new: true }); expect(departure).toEqual({ room: updatedRoom, adminChanged: true }); expect(mirrorRoomChanges).toHaveBeenCalledWith(updatedRoom, { @@ -309,6 +314,30 @@ describe('room security helpers', () => { }); }); + it('advances an active user presence revision atomically', async () => { + const updatedRoom = { id: 'room-one' }; + const query = { + populate: jest.fn(() => query), + then: (resolve, reject) => Promise.resolve(updatedRoom).then(resolve, reject), + }; + const model = { + findOneAndUpdate: jest.fn(() => query), + }; + const room = { + _id: 'room-one', + constructor: model, + }; + + await expect(Room.methods.advancePresenceRevision.call(room, 101)).resolves.toBe(updatedRoom); + + expect(model.findOneAndUpdate).toHaveBeenCalledWith({ + _id: room._id, + 'inRoom.101': true, + }, { + $inc: { 'presenceRevision.101': 1 }, + }, { new: true }); + }); + it('persists and announces the owner reclaiming admin controls', async () => { const owner = { id: 101 }; const room = { diff --git a/server/socket/namespaces/rooms.js b/server/socket/namespaces/rooms.js index 036eac6..74d4207 100644 --- a/server/socket/namespaces/rooms.js +++ b/server/socket/namespaces/rooms.js @@ -88,6 +88,7 @@ module.exports = (io, middlewares) => { roomId: room._id, userId: user.id, membershipRevision: room.membershipRevision, + presenceRevision: room.presenceRevision.get(user.id.toString()), leaveReason: 'disconnect', }))); } @@ -126,13 +127,14 @@ module.exports = (io, middlewares) => { } async function finalizeUserDeparture({ - roomId, userId, connectionId, leaveReason, membershipRevision, + roomId, userId, connectionId, leaveReason, membershipRevision, presenceRevision, }) { const userKey = userId.toString(); const room = await fetchRoom(roomId); if (!room || !room.inRoom.get(userKey) - || room.membershipRevision !== membershipRevision) { + || room.membershipRevision !== membershipRevision + || room.presenceRevision.get(userKey) !== presenceRevision) { return false; } @@ -143,7 +145,11 @@ module.exports = (io, middlewares) => { // The membership revision is the departure generation. A failed claim is // terminal: retrying it after a rejoin would remove the new membership. - const departure = await room.dropUserAtomically({ id: userId }, membershipRevision); + const departure = await room.dropUserAtomically( + { id: userId }, + membershipRevision, + presenceRevision, + ); if (!departure) { return false; } @@ -372,6 +378,7 @@ module.exports = (io, middlewares) => { userId: socket.userId, connectionId: socket.id, membershipRevision: socket.room.membershipRevision, + presenceRevision: socket.room.presenceRevision.get(socket.userId.toString()), leaveReason, }); } catch (e) { @@ -423,18 +430,42 @@ module.exports = (io, middlewares) => { }); } - const r = await activeRoom.addUser(socket.user, spectating, sendAdminUpdate); + const presentRoom = await activeRoom.advancePresenceRevision(socket.userId); + if (presentRoom) { + socket.room = presentRoom; + await metrics.beginRoomVisit({ + room: presentRoom, + userId: socket.userId, + connectionId: socket.id, + activeUserCount: presentRoom.usersLength, + }); + return cb(null, joinRoomMask(presentRoom)); + } + const currentRoom = await fetchRoom(room._id); + if (!currentRoom) { + return cb({ + statusCode: 404, + message: 'Room no longer exists', + }); + } + const r = await currentRoom.addUser(socket.user, spectating, sendAdminUpdate); if (!r) { - // The user is already present in persisted room state. - socket.room = activeRoom; + const fencedRoom = await currentRoom.advancePresenceRevision(socket.userId); + if (!fencedRoom) { + return cb({ + statusCode: 409, + message: 'Room membership changed while joining', + }); + } + socket.room = fencedRoom; await metrics.beginRoomVisit({ - room: activeRoom, + room: fencedRoom, userId: socket.userId, connectionId: socket.id, - activeUserCount: activeRoom.usersLength, + activeUserCount: fencedRoom.usersLength, }); - return cb(null, joinRoomMask(activeRoom)); + return cb(null, joinRoomMask(fencedRoom)); } socket.room = r; @@ -1072,6 +1103,7 @@ module.exports = (io, middlewares) => { userId: socket.userId, connectionId: socket.id, membershipRevision: socket.room.membershipRevision, + presenceRevision: socket.room.presenceRevision.get(socket.userId.toString()), leaveReason: 'disconnect', }); } else if (socket.room) { diff --git a/server/socket/namespaces/rooms.test.js b/server/socket/namespaces/rooms.test.js index 16148f0..701165c 100644 --- a/server/socket/namespaces/rooms.test.js +++ b/server/socket/namespaces/rooms.test.js @@ -58,26 +58,34 @@ const queryResult = (value) => { return query; }; -const makeRoom = (overrides = {}) => ({ - _id: 'room-one', - accessCode: 'PRIVATE', - private: true, - password: 'bcrypt-hash-one', - type: 'normal', - owner: { id: 101 }, - admin: { id: 101 }, - users: [], - usersInRoom: [], - inRoom: new Map(), - banned: new Map(), - registered: new Map(), - membershipRevision: 7, - requireRevealedIdentity: false, - usersLength: 1, - authenticate: jest.fn().mockResolvedValue(true), - addUser: jest.fn().mockResolvedValue(false), - ...overrides, -}); +const makeRoom = (overrides = {}) => { + const room = { + _id: 'room-one', + accessCode: 'PRIVATE', + private: true, + password: 'bcrypt-hash-one', + type: 'normal', + owner: { id: 101 }, + admin: { id: 101 }, + users: [], + usersInRoom: [], + inRoom: new Map(), + banned: new Map(), + registered: new Map(), + membershipRevision: 7, + presenceRevision: new Map([['101', 7]]), + requireRevealedIdentity: false, + usersLength: 1, + authenticate: jest.fn().mockResolvedValue(true), + addUser: jest.fn().mockResolvedValue(false), + advancePresenceRevision: jest.fn(), + }; + Object.assign(room, overrides); + if (!overrides.advancePresenceRevision) { + room.advancePresenceRevision.mockResolvedValue(room); + } + return room; +}; const makeSocket = ({ id, session, userId = 202, @@ -230,6 +238,7 @@ describe('private room namespace joins', () => { onAdminChange(room); return room; }); + room.advancePresenceRevision.mockResolvedValue(null); room.edit = jest.fn().mockResolvedValue(room); const connect = setup(new Map([[room._id, room]])); const socket = await connect(makeSocket({ @@ -380,6 +389,7 @@ describe('room namespace departures and moderation', () => { userId: 101, connectionId: 'host-socket', membershipRevision: room.membershipRevision, + presenceRevision: room.presenceRevision.get('101'), leaveReason: 'explicit', }); @@ -414,6 +424,7 @@ describe('room namespace departures and moderation', () => { userId: 101, connectionId: 'winner-socket', membershipRevision: 7, + presenceRevision: 7, leaveReason: 'explicit', })).resolves.toBe(true); @@ -422,6 +433,7 @@ describe('room namespace departures and moderation', () => { admin: { id: 101 }, inRoom: new Map([['101', true]]), membershipRevision: 9, + presenceRevision: new Map([['101', 9]]), }; persistedRoom = rejoinedRoom; @@ -430,6 +442,7 @@ describe('room namespace departures and moderation', () => { userId: 101, connectionId: 'loser-socket', membershipRevision: 7, + presenceRevision: 7, leaveReason: 'explicit', })).resolves.toBe(false); @@ -442,6 +455,86 @@ describe('room namespace departures and moderation', () => { expect(connect.roomChannel.emit).toHaveBeenCalledWith(Protocol.USER_LEFT, 101); }); + it('fences an active duplicate join before an older departure can finalize', async () => { + const room = makeRoom({ + private: false, + password: null, + inRoom: new Map([['101', true]]), + }); + const fencedRoom = { + ...room, + presenceRevision: new Map([['101', 8]]), + }; + room.advancePresenceRevision.mockResolvedValue(fencedRoom); + const connect = setup(new Map([[room._id, room]])); + const socket = await connect(makeSocket({ id: 'new-tab', session: {}, userId: 101 })); + + const acknowledgment = await joinRoom(socket, { id: room._id }); + + expect(acknowledgment).toHaveBeenCalledWith( + null, + expect.objectContaining({ _id: room._id }), + ); + expect(room.advancePresenceRevision).toHaveBeenCalledWith(101); + + Room.findById.mockImplementation(() => queryResult(fencedRoom)); + const reconnectGrace = createReconnectGrace.mock.results[0].value; + + await expect(reconnectGrace.finalizeDeparture({ + roomId: room._id, + userId: 101, + connectionId: 'old-tab', + membershipRevision: 7, + presenceRevision: 7, + leaveReason: 'explicit', + })).resolves.toBe(false); + + expect(room.dropUserAtomically).toBeUndefined(); + expect(fencedRoom.inRoom.get('101')).toBe(true); + expect(fencedRoom.admin).toEqual({ id: 101 }); + }); + + it('joins normally when a departure wins before the duplicate presence fence', async () => { + const room = makeRoom({ + private: false, + password: null, + inRoom: new Map([['101', true]]), + }); + room.advancePresenceRevision.mockResolvedValue(null); + const rejoinedRoom = { + ...room, + inRoom: new Map([['101', true]]), + membershipRevision: 8, + presenceRevision: new Map([['101', 8]]), + }; + const departedRoom = { + ...room, + inRoom: new Map([['101', false]]), + addUser: jest.fn().mockResolvedValue(rejoinedRoom), + }; + const connect = setup(new Map([[room._id, room]])); + let fetchCount = 0; + Room.findById.mockImplementation(() => { + const fetchedRoom = fetchCount === 0 ? room : departedRoom; + fetchCount += 1; + return queryResult(fetchedRoom); + }); + const socket = await connect(makeSocket({ id: 'new-tab', session: {}, userId: 101 })); + + const acknowledgment = await joinRoom(socket, { id: room._id }); + + expect(departedRoom.addUser).toHaveBeenCalledWith( + socket.user, + undefined, + expect.any(Function), + ); + expect(acknowledgment).toHaveBeenCalledWith( + null, + expect.objectContaining({ _id: room._id }), + ); + expect(socket.room).toBe(rejoinedRoom); + }); + it('finalizes one departure when two tabs explicitly leave together', async () => { const room = makeRoom({ private: false, From e7d6859ed52e36653fedcd77bd65b3d4b362dfc3 Mon Sep 17 00:00:00 2001 From: Cailyn Sinclair Date: Sun, 12 Jul 2026 21:53:32 -0700 Subject: [PATCH 6/6] Reauthorize recovered room joins Validate the current room state after a departure wins the duplicate-join race, preventing bans or changed access rules from being bypassed. --- server/socket/namespaces/rooms.js | 47 ++++++++++++++++++++++++-- server/socket/namespaces/rooms.test.js | 32 ++++++++++++++++++ 2 files changed, 77 insertions(+), 2 deletions(-) diff --git a/server/socket/namespaces/rooms.js b/server/socket/namespaces/rooms.js index 74d4207..e430b4d 100644 --- a/server/socket/namespaces/rooms.js +++ b/server/socket/namespaces/rooms.js @@ -386,7 +386,12 @@ module.exports = (io, middlewares) => { } } - async function joinRoom(room, cb, spectating) { + async function joinRoom( + room, + cb, + spectating, + { password, reauthorizeOnRecovery = false } = {}, + ) { if (socket.roomId) { if (String(socket.roomId) === String(room._id)) { socket.room = socket.room || room; @@ -449,6 +454,41 @@ module.exports = (io, middlewares) => { message: 'Room no longer exists', }); } + if (reauthorizeOnRecovery) { + const rejectRecoveryJoin = (error) => { + socket.leave(room.accessCode); + socket.leave(encodeUserRoom(socket.userId, room._id)); + delete socket.room; + delete socket.roomId; + return cb(error); + }; + const userKey = socket.userId.toString(); + if (!isRoomTypeEnabled(currentRoom.type)) { + return rejectRecoveryJoin({ + statusCode: 403, + message: 'Grand Prix rooms are disabled', + }); + } + if (currentRoom.private && (!password || !(await currentRoom.authenticate(password)))) { + return rejectRecoveryJoin({ + statusCode: 403, + message: 'Invalid password', + }); + } + if (currentRoom.banned.get(userKey)) { + return rejectRecoveryJoin({ + statusCode: 401, + message: 'Banned', + banned: true, + }); + } + if (currentRoom.requireRevealedIdentity && !socket.user.showWCAID) { + return rejectRecoveryJoin({ + statusCode: 403, + message: 'Must be showing WCA Identity to join room.', + }); + } + } const r = await currentRoom.addUser(socket.user, spectating, sendAdminUpdate); if (!r) { const fencedRoom = await currentRoom.advancePresenceRevision(socket.userId); @@ -564,7 +604,10 @@ module.exports = (io, middlewares) => { }, room); } - return await joinRoom(room, acknowledgment, spectating); + return await joinRoom(room, acknowledgment, spectating, { + password, + reauthorizeOnRecovery: true, + }); } catch (e) { logger.error(e); return rejectJoin('internal_error', { diff --git a/server/socket/namespaces/rooms.test.js b/server/socket/namespaces/rooms.test.js index 701165c..eb31f07 100644 --- a/server/socket/namespaces/rooms.test.js +++ b/server/socket/namespaces/rooms.test.js @@ -535,6 +535,38 @@ describe('room namespace departures and moderation', () => { expect(socket.room).toBe(rejoinedRoom); }); + it('does not re-add a user banned while a duplicate join recovers from departure', async () => { + const room = makeRoom({ + private: false, + password: null, + inRoom: new Map([['101', true]]), + }); + room.advancePresenceRevision.mockResolvedValue(null); + const departedAndBannedRoom = { + ...room, + inRoom: new Map([['101', false]]), + banned: new Map([['101', true]]), + addUser: jest.fn(), + }; + const connect = setup(new Map([[room._id, room]])); + let fetchCount = 0; + Room.findById.mockImplementation(() => { + const fetchedRoom = fetchCount === 0 ? room : departedAndBannedRoom; + fetchCount += 1; + return queryResult(fetchedRoom); + }); + const socket = await connect(makeSocket({ id: 'new-tab', session: {}, userId: 101 })); + + const acknowledgment = await joinRoom(socket, { id: room._id }); + + expect(departedAndBannedRoom.addUser).not.toHaveBeenCalled(); + expect(acknowledgment).toHaveBeenCalledWith(expect.objectContaining({ + statusCode: 401, + banned: true, + })); + expect(socket.roomId).toBeUndefined(); + }); + it('finalizes one departure when two tabs explicitly leave together', async () => { const room = makeRoom({ private: false,