fix(socketio): don't kick authenticated duplicate-author sessions (#7656)

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}` +

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) {}