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..3b1297e --- /dev/null +++ b/docs/room-ownership.md @@ -0,0 +1,66 @@ +# 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: 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. + +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 +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. + +## 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`. + +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 +solely because owner deletion authority survives departure. diff --git a/server/models/room.js b/server/models/room.js index 88881ed..275f102 100644 --- a/server/models/room.js +++ b/server/models/room.js @@ -7,6 +7,33 @@ 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 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 = { // }; @@ -87,6 +114,17 @@ 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, + }, + presenceRevision: { + type: Map, + of: Number, + default: {}, + }, admin: { type: mongoose.Schema.Types.ObjectId, ref: 'User' }, owner: { type: mongoose.Schema.Types.ObjectId, ref: 'User' }, type: { @@ -187,6 +225,8 @@ 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); @@ -210,10 +250,10 @@ 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; + advancePresenceRevisionFor(this, user.id.toString()); - if (updateAdmin) { - await this.updateAdminIfNeeded(updateAdmin); - } + await this.updateAdminIfNeeded(updateAdmin); if (this.usersInRoom.length === 0 && this.type === 'normal') { await this.updateStale(true); @@ -222,6 +262,105 @@ Room.methods.dropUser = async function (user, updateAdmin) { return this.save(); }; +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; + const presenceRevision = presenceRevisionFor(this, userKey); + if (!this.inRoom.get(userKey) + || expectedMembershipRevision !== membershipRevision + || expectedPresenceRevision !== presenceRevision) { + 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 condition = { + _id: this._id, + [`inRoom.${userKey}`]: true, + }; + const revisionConditions = []; + if (membershipRevision > 0) { + condition.membershipRevision = membershipRevision; + } else { + 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 = { + $set: { + [`inRoom.${userKey}`]: false, + [`waitingFor.${userKey}`]: false, + admin: nextAdmin ? nextAdmin._id || nextAdmin : null, + }, + $inc: { + membershipRevision: 1, + [`presenceRevision.${userKey}`]: 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.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 }); @@ -316,25 +455,20 @@ 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(); +Room.methods.updateAdminIfNeeded = async function (cb) { + const nextAdmin = selectRoomAdmin(this); + if (sameUser(this.admin, nextAdmin) || (!this.admin && !nextAdmin)) { + return this; } - 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); - } - - if (!this.admin || this.admin.id !== this.usersInRoom[0].id) { - const { usersInRoom } = this; - - // eslint-disable-next-line prefer-destructuring - this.admin = usersInRoom[0]; - return this.save().then(cb); + this.admin = nextAdmin; + const room = await this.save(); + // 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; }; const collectPostgresChanges = (room) => { @@ -411,3 +545,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..ea83799 100644 --- a/server/models/room.test.js +++ b/server/models/room.test.js @@ -4,11 +4,19 @@ const bcrypt = require('bcrypt'); const mongoose = require('mongoose'); const { generateScramble } = require('letscube-scrambles'); -const { collectPostgresChanges, Room } = require('./room'); +const { mirrorRoomChanges } = require('../postgres/dualWrite'); +const { + collectPostgresChanges, + Room, + selectRoomAdmin, +} = require('./room'); jest.mock('letscube-scrambles', () => ({ generateScramble: jest.fn(), })); +jest.mock('../postgres/dualWrite', () => ({ + mirrorRoomChanges: jest.fn().mockResolvedValue(undefined), +})); const RoomModel = mongoose.model('RoomPostgresChangesTest', Room); @@ -166,6 +174,188 @@ 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('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('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 room = { + _id: 'room-one', + owner, + admin: owner, + users: [owner, nextAdmin], + inRoom: new Map([['101', true], ['202', true]]), + type: 'normal', + membershipRevision: 7, + presenceRevision: new Map([['101', 7]]), + constructor: model, + }; + + const departure = await Room.methods.dropUserAtomically.call(room, owner); + + expect(model.findOneAndUpdate).toHaveBeenCalledWith({ + _id: room._id, + 'inRoom.101': true, + membershipRevision: 7, + 'presenceRevision.101': 7, + }, { + $set: { + 'inRoom.101': false, + 'waitingFor.101': false, + admin: nextAdmin._id, + }, + $inc: { + membershipRevision: 1, + 'presenceRevision.101': 1, + }, + }, { new: true }); + expect(departure).toEqual({ room: updatedRoom, adminChanged: true }); + expect(mirrorRoomChanges).toHaveBeenCalledWith(updatedRoom, { + attempts: [], + participantUserIds: ['101'], + syncAllParticipants: false, + syncRoomOwners: true, + }); + }); + + 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 = { + 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..e430b4d 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'); @@ -83,6 +87,8 @@ module.exports = (io, middlewares) => { return rooms.flatMap((room) => room.usersInRoom.map((user) => ({ roomId: room._id, userId: user.id, + membershipRevision: room.membershipRevision, + presenceRevision: room.presenceRevision.get(user.id.toString()), leaveReason: 'disconnect', }))); } @@ -114,13 +120,21 @@ 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, + roomId, userId, connectionId, leaveReason, membershipRevision, presenceRevision, }) { - const room = await fetchRoom(roomId); const userKey = userId.toString(); - if (!room || !room.inRoom.get(userKey)) { + const room = await fetchRoom(roomId); + if (!room || !room.inRoom.get(userKey) + || room.membershipRevision !== membershipRevision + || room.presenceRevision.get(userKey) !== presenceRevision) { return false; } @@ -129,9 +143,21 @@ module.exports = (io, middlewares) => { return false; } - const updatedRoom = await room.dropUser({ id: userId }, (_room) => { - ns().in(room.accessCode).emit(Protocol.UPDATE_ADMIN, _room.admin); - }); + // 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, + presenceRevision, + ); + if (!departure) { + return false; + } + + const { room: updatedRoom, adminChanged } = departure; + if (adminChanged) { + sendAdminUpdate(updatedRoom); + } await metrics.endRoomVisit({ room: updatedRoom, @@ -323,7 +349,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, @@ -351,6 +377,8 @@ module.exports = (io, middlewares) => { roomId: socket.roomId, userId: socket.userId, connectionId: socket.id, + membershipRevision: socket.room.membershipRevision, + presenceRevision: socket.room.presenceRevision.get(socket.userId.toString()), leaveReason, }); } catch (e) { @@ -358,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; @@ -402,20 +435,77 @@ module.exports = (io, middlewares) => { }); } - const r = await activeRoom.addUser(socket.user, spectating, (_room) => { - ns().in(_room.accessCode).emit(Protocol.UPDATE_ADMIN, _room.admin); - }); + 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', + }); + } + 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) { - // 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; @@ -514,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', { @@ -756,7 +849,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 +936,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, @@ -916,6 +1009,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 }); @@ -950,6 +1052,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); @@ -1034,6 +1145,8 @@ module.exports = (io, middlewares) => { roomId: socket.roomId, userId: socket.userId, connectionId: socket.id, + membershipRevision: socket.room.membershipRevision, + presenceRevision: socket.room.presenceRevision.get(socket.userId.toString()), leaveReason: 'disconnect', }); } else if (socket.room) { @@ -1054,8 +1167,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 e24cb4c..eb31f07 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(), })), @@ -55,25 +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(), - 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, @@ -103,13 +115,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 +138,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 +222,75 @@ 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.advancePresenceRevision.mockResolvedValue(null); + 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', () => { @@ -273,3 +358,280 @@ 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.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: {} })); + const reconnectGrace = createReconnectGrace.mock.results[0].value; + + await reconnectGrace.finalizeDeparture({ + roomId: room._id, + userId: 101, + connectionId: 'host-socket', + membershipRevision: room.membershipRevision, + presenceRevision: room.presenceRevision.get('101'), + leaveReason: 'explicit', + }); + + expect(connect.roomChannel.emit).not.toHaveBeenCalledWith(Protocol.UPDATE_ADMIN, null); + expect(connect.roomChannel.emit).toHaveBeenCalledWith(Protocol.USER_LEFT, 101); + }); + + it('does not let a losing departure remove a later rejoin', async () => { + const room = makeRoom({ + private: false, + password: null, + inRoom: new Map([['101', true]]), + }); + const updatedRoom = { + ...room, + inRoom: new Map([['101', false]]), + membershipRevision: 8, + usersLength: 0, + doneWithScramble: jest.fn().mockReturnValue(false), + }; + room.dropUserAtomically = jest.fn().mockResolvedValue({ + room: updatedRoom, + adminChanged: false, + }); + const connect = setup(new Map([[room._id, room]])); + 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, + presenceRevision: 7, + leaveReason: 'explicit', + })).resolves.toBe(true); + + const rejoinedRoom = { + ...room, + admin: { id: 101 }, + inRoom: new Map([['101', true]]), + membershipRevision: 9, + presenceRevision: new Map([['101', 9]]), + }; + persistedRoom = rejoinedRoom; + + await expect(reconnectGrace.finalizeDeparture({ + roomId: room._id, + userId: 101, + connectionId: 'loser-socket', + membershipRevision: 7, + presenceRevision: 7, + leaveReason: 'explicit', + })).resolves.toBe(false); + + 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); + }); + + 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('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, + 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, + membershipRevision: room.membershipRevision, + 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, + })); + }); +});