diff --git a/src/node/handler/PadMessageHandler.ts b/src/node/handler/PadMessageHandler.ts index 5fe717615b9..5a0a4c8544b 100644 --- a/src/node/handler/PadMessageHandler.ts +++ b/src/node/handler/PadMessageHandler.ts @@ -239,22 +239,32 @@ exports.handleDisconnect = async (socket:any) => { ` authorID:${session.author}` + (user && user.username ? ` username:${user.username}` : '')); /* eslint-enable prefer-template */ - socket.broadcast.to(session.padId).emit('message', { - type: 'COLLABROOM', - data: { - type: 'USER_LEAVE', - userInfo: { - colorId: await authorManager.getAuthorColorId(session.author), - userId: session.author, + // Client presence is keyed by authorID. With the #7656 fix, multiple sockets + // can share an authorID (same authenticated identity across windows/devices), + // so emitting USER_LEAVE on every socket disconnect would drop the author + // from presence even when another socket of theirs is still connected. Only + // broadcast — and only run the userLeave hook — when the *last* socket for + // this author leaves the pad. + const isLastSocketForAuthor = !_getRoomSockets(session.padId).some( + (s: any) => sessioninfos[s.id]?.author === session.author); + if (isLastSocketForAuthor) { + socket.broadcast.to(session.padId).emit('message', { + type: 'COLLABROOM', + data: { + type: 'USER_LEAVE', + userInfo: { + colorId: await authorManager.getAuthorColorId(session.author), + userId: session.author, + }, }, - }, - }); - await hooks.aCallAll('userLeave', { - ...session, // For backwards compatibility. - authorId: session.author, - readOnly: session.readonly, - socket, - }); + }); + await hooks.aCallAll('userLeave', { + ...session, // For backwards compatibility. + authorId: session.author, + readOnly: session.readonly, + socket, + }); + } }; @@ -1020,22 +1030,29 @@ const handleClientReady = async (socket:any, message: ClientReadyMessage) => { // Check if the user has disconnected during any of the above awaits. if (sessionInfo !== sessioninfos[socket.id]) throw new Error('client disconnected'); - // Check if this author is already on the pad, if yes, kick the other sessions! - const roomSockets = _getRoomSockets(pad.id); + const {session: {user} = {}} = socket.client.request as SocketClientRequest; - for (const otherSocket of roomSockets) { - // The user shouldn't have joined the room yet, but check anyway just in case. - if (otherSocket.id === socket.id) continue; - const sinfo = sessioninfos[otherSocket.id]; - if (sinfo && sinfo.author === sessionInfo.author) { - // fix user's counter, works on page refresh or if user closes browser window and then rejoins - sessioninfos[otherSocket.id] = {}; - otherSocket.leave(sessionInfo.padId); - otherSocket.emit('message', {disconnect: 'userdup'}); + // The duplicate-author kick exists because cookie-derived authorIDs are + // per-browser, so "same authorID, same pad" historically meant "stale tab in + // the same browser" — see #7656. Authenticated sessions (req.session.user + // set, e.g. via basic auth, SSO, or a getAuthorId plugin hook) carry a + // stable identity across windows and devices, so concurrent same-author + // sessions are legitimate and must not be kicked. + const roomSockets = _getRoomSockets(pad.id); + if (user == null) { + for (const otherSocket of roomSockets) { + // The user shouldn't have joined the room yet, but check anyway just in case. + if (otherSocket.id === socket.id) continue; + const sinfo = sessioninfos[otherSocket.id]; + if (sinfo && sinfo.author === sessionInfo.author) { + // fix user's counter, works on page refresh or if user closes browser window and then rejoins + sessioninfos[otherSocket.id] = {}; + otherSocket.leave(sessionInfo.padId); + otherSocket.emit('message', {disconnect: 'userdup'}); + } } } - const {session: {user} = {}} = socket.client.request as SocketClientRequest; /* eslint-disable prefer-template -- it doesn't support breaking across multiple lines */ accessLogger.info(`[${pad.head > 0 ? 'ENTER' : 'CREATE'}]` + ` pad:${sessionInfo.padId}` + diff --git a/src/tests/backend/specs/socketio.ts b/src/tests/backend/specs/socketio.ts index b235974aa71..fa7a7dd2723 100644 --- a/src/tests/backend/specs/socketio.ts +++ b/src/tests/backend/specs/socketio.ts @@ -324,6 +324,114 @@ describe(__filename, function () { }); }); + describe('Duplicate-author handling (#7656)', function () { + let socketA: any; + let socketB: any; + + afterEach(async function () { + for (const s of [socketA, socketB]) if (s) s.close(); + socketA = null; + socketB = null; + // The outer afterEach only knows about the singleton `socket`. Null it so + // it doesn't try to close one of ours twice. + socket = null; + }); + + // Records `{disconnect: ...}` payloads delivered to a socket so a test can + // assert whether the userdup kick fired. + const watchDisconnects = (s: any): string[] => { + const seen: string[] = []; + s.on('message', (msg: any) => { if (msg && msg.disconnect) seen.push(msg.disconnect); }); + return seen; + }; + + it('cookie identity: same-author second socket kicks the first (regression)', async function () { + const res = await agent.get('/p/pad').expect(200); + socketA = await common.connect(res); + assert.equal((await common.handshake(socketA, 'pad')).type, 'CLIENT_VARS'); + const seen = watchDisconnects(socketA); + + // Same cookie => same author token => same authorID. This is the original + // "stale tab in the same browser" case the kick was designed for. + socketB = await common.connect(res); + assert.equal((await common.handshake(socketB, 'pad')).type, 'CLIENT_VARS'); + // Let the kick emit drain. + await new Promise((r) => setTimeout(r, 200)); + assert.deepEqual(seen, ['userdup']); + }); + + it('authenticated identity: second socket does NOT kick the first', async function () { + settings.requireAuthentication = true; + const res = await agent.get('/p/pad').auth('user', 'user-password').expect(200); + socketA = await common.connect(res); + assert.equal((await common.handshake(socketA, 'pad')).type, 'CLIENT_VARS'); + const seen = watchDisconnects(socketA); + + socketB = await common.connect(res); + assert.equal((await common.handshake(socketB, 'pad')).type, 'CLIENT_VARS'); + await new Promise((r) => setTimeout(r, 200)); + assert.deepEqual(seen, []); + }); + + // Records USER_LEAVE userIds so a test can assert whether other clients + // were told the author went offline. + const watchUserLeaves = (s: any): string[] => { + const seen: string[] = []; + s.on('message', (msg: any) => { + if (msg?.type === 'COLLABROOM' && msg?.data?.type === 'USER_LEAVE') { + seen.push(msg.data.userInfo?.userId); + } + }); + return seen; + }; + + it('authenticated identity: closing one socket does NOT remove the author for the other', async function () { + // Two authenticated sockets sharing one identity (same authorID). When + // socketA closes, presence-key clients keyed on authorID would drop the + // author entirely if USER_LEAVE were broadcast — but socketB is still + // online. The server must only emit USER_LEAVE when the *last* socket + // for that author leaves. + settings.requireAuthentication = true; + const res = await agent.get('/p/pad').auth('user', 'user-password').expect(200); + socketA = await common.connect(res); + const cvA: any = await common.handshake(socketA, 'pad'); + const authorIdA = cvA.data.userId; + socketB = await common.connect(res); + const cvB: any = await common.handshake(socketB, 'pad'); + assert.equal(cvB.data.userId, authorIdA, 'precondition: same author'); + + const leaves = watchUserLeaves(socketB); + socketA.close(); + socketA = null; + // Give the server a beat to broadcast. + await new Promise((r) => setTimeout(r, 300)); + assert.deepEqual(leaves, [], + 'remaining same-author socket should not see USER_LEAVE for itself'); + }); + + it('different authors: closing one socket DOES emit USER_LEAVE for the other (regression)', async function () { + // socketA and socketB are different anonymous browsers (separate cookie + // jars => separate authorIDs). Closing socketA must still tell socketB + // that authorA left. + const supertest = require('supertest'); + const browserA = supertest(common.baseUrl); + const browserB = supertest(common.baseUrl); + const resA = await browserA.get('/p/pad').expect(200); + const resB = await browserB.get('/p/pad').expect(200); + socketA = await common.connect(resA); + const cvA: any = await common.handshake(socketA, 'pad'); + socketB = await common.connect(resB); + const cvB: any = await common.handshake(socketB, 'pad'); + assert.notEqual(cvA.data.userId, cvB.data.userId, 'precondition: different authors'); + + const leaves = watchUserLeaves(socketB); + socketA.close(); + socketA = null; + await new Promise((r) => setTimeout(r, 300)); + assert.deepEqual(leaves, [cvA.data.userId]); + }); + }); + describe('SocketIORouter.js', function () { const Module = class { setSocketIO(io:any) {}