Fix premature note cleanup on error

Connection forbidden errors cause cleanup of note state without first
checking if other clients are still connected to the note. This leads
to inconsistent pad content and changes not being saved properly.

This change reverts parts of 725e982 (Fix realtime on forbidden not clean
up properly ...). The call to `interruptConnection()` on permission errors
is redundant, since `failConnection()` and `disconnect()` already perform
all required cleanup in this case. The other call to `interruptConnection()`
only happens when a client (the first client for a note) disconnects while
the note is being loaded from the database. It is refactored for clarity.

Fixes #3894

Co-authored-by: David Mehren <git@herrmehren.de>
Signed-off-by: Julian Rother <julian@jrother.eu>

lib/realtime.js

@@ -363,13 +363,6 @@ function connectNextSocket () {
   }, 1)
 }
 
-function interruptConnection (socket, noteId, socketId) {
-  if (notes[noteId]) delete notes[noteId]
-  if (users[socketId]) delete users[socketId]
-  if (socket) { clearSocketQueue(connectionSocketQueue, socket) } else { connectionSocketQueue.shift() }
-  connectNextSocket()
-}
-
 function checkViewPermission (req, note) {
   if (note.permission === 'private') {
     if (req.user && req.user.logged_in && req.user.id === note.owner) { return true } else { return false }
@@ -386,13 +379,8 @@ let isDisconnectBusy = false
 const disconnectSocketQueue = []
 
 function finishConnection (socket, noteId, socketId) {
-  // if no valid info provided will drop the client
-  if (!socket || !notes[noteId] || !users[socketId]) {
-    return interruptConnection(socket, noteId, socketId)
-  }
   // check view permission
   if (!checkViewPermission(socket.request, notes[noteId])) {
-    interruptConnection(socket, noteId, socketId)
     return failConnection(403, 'connection forbidden', socket)
   }
   const note = notes[noteId]
@@ -460,6 +448,13 @@ function startConnection (socket) {
       },
       include
     }).then(function (note) {
+      // if client disconnected while we waited for the note, disconnect() cleaned up users[socket.id]
+      if (!users[socket.id]) {
+        clearSocketQueue(connectionSocketQueue, socket)
+        connectNextSocket()
+        return
+      }
+
       if (!note) {
         return failConnection(404, 'note not found', socket)
       }

public/docs/release-notes.md

@@ -10,6 +10,9 @@ You now need Node 16 to run HedgeDoc. We don't support more recent versions of N
 - Allow setting of `documentMaxLength` via `CMD_DOCUMENT_MAX_LENGTH` environment variable.
 - Add dedicated healthcheck endpoint at /_health that is less resource intensive than /status.
 
+### Bugfixes
+- Fix that permission errors can break existing connections to a note, causing inconsistent note content and changes not being saved
+
 ## <i class="fa fa-tag"></i> 1.9.7 <i class="fa fa-calendar-o"></i> 2023-02-19
 
 ### Bugfixes