From 1bb44098df5b1292e058b6dba5a695946805990a Mon Sep 17 00:00:00 2001
From: Richard Hansen <rhansen@rhansen.org>
Date: Thu, 10 Sep 2020 13:27:59 -0400
Subject: [PATCH] PadMessageHandler: Move handleMessage hooks after access
 check

Move the handleMessageSecurity and handleMessage hooks after the call
to securityManager.checkAccess.

Benefits:

  * A handleMessage plugin can safely assume the message will be
    handled unless the plugin itself drops the message, so it doesn't
    need to repeat the access checks done by the `handleMessage`
    function.
  * This paves the way for a future enhancement: pass the author ID to
    the hooks.

Note: The handleMessageSecurity hook is broken in several ways:

  * The hook result is ignored for `CLIENT_READY` and `SWITCH_TO_PAD`
    messages because the `handleClientReady` function overwrites the
    hook result. This causes the client to receive client vars with
    `readonly` set to true, which causes the client to display an
    immutable pad even though the pad is technically writable.
  * The formatting toolbar buttons are removed for read-only pads
    before the handleMessageSecurity hook even runs.
  * It is awkwardly named: Without reading the documentation, how is
    one supposed to know that "handle message security" actually means
    "grant one-time write access to a read-only pad"?
  * It is called for every message even though calls after a
    `CLIENT_READY` or `SWITCH_TO_PAD` are mostly pointless.
  * Why would anyone want to grant write access when the user visits a
    read-only pad URL? The user should just visit the writable pad URL
    instead.
  * Why would anyone want to grant write access that only lasts for a
    single socket.io connection?
  * There are better ways to temporarily grant write access (e.g., the
    authorize hook).
  * This hook is inviting bugs because it breaks a core assumption
    about `/p/r.*` URLs.

I think the hook should be deprecated and eventually removed.
---
 doc/api/hooks_server-side.md          |  7 ------
 src/node/handler/PadMessageHandler.js | 33 ++++++++++++---------------
 2 files changed, 15 insertions(+), 25 deletions(-)

diff --git a/doc/api/hooks_server-side.md b/doc/api/hooks_server-side.md
index 0a4f95181..dd0b8599e 100644
--- a/doc/api/hooks_server-side.md
+++ b/doc/api/hooks_server-side.md
@@ -399,9 +399,6 @@ The handleMessage function must return a Promise. If the Promise resolves to
 `null`, the message is dropped. Returning `callback(value)` will return a
 Promise that is resolved to `value`.
 
-**WARNING:** handleMessage is called for every message, even if the client is
-not authorized to send the message. It is up to the plugin to check permissions.
-
 Examples:
 
 ```
@@ -444,10 +441,6 @@ The handleMessageSecurity function must return a Promise. If the Promise
 resolves to `true`, write access is granted as described above. Returning
 `callback(value)` will return a Promise that is resolved to `value`.
 
-**WARNING:** handleMessageSecurity is called for every message, even if the
-client is not authorized to send the message. It is up to the plugin to check
-permissions.
-
 Examples:
 
 ```
diff --git a/src/node/handler/PadMessageHandler.js b/src/node/handler/PadMessageHandler.js
index f6d72764b..e311e9dbf 100644
--- a/src/node/handler/PadMessageHandler.js
+++ b/src/node/handler/PadMessageHandler.js
@@ -199,23 +199,6 @@ exports.handleMessage = async function(client, message)
     return;
   }
 
-  // Allow plugins to bypass the readonly message blocker
-  if ((await hooks.aCallAll('handleMessageSecurity', {client, message})).some((w) => w === true)) {
-    thisSession.readonly = false;
-  }
-
-  // Call handleMessage hook. If a plugin returns null, the message will be dropped. Note that for
-  // all messages handleMessage will be called, even if the client is not authorized
-  if ((await hooks.aCallAll('handleMessage', {client, message})).some((m) => m === null)) {
-    return;
-  }
-
-  // Drop the message if the client disconnected while the hooks were running.
-  if (sessioninfos[client.id] !== thisSession) {
-    messageLogger.warn("Dropping message from a connection that has gone away.")
-    return;
-  }
-
   if (message.type === "CLIENT_READY") {
     // client tried to auth for the first time (first msg from the client)
     createSessionInfoAuth(thisSession, message);
@@ -245,7 +228,21 @@ exports.handleMessage = async function(client, message)
     return;
   }
 
-  // access was granted
+  // Allow plugins to bypass the readonly message blocker
+  if ((await hooks.aCallAll('handleMessageSecurity', {client, message})).some((w) => w === true)) {
+    thisSession.readonly = false;
+  }
+
+  // Call handleMessage hook. If a plugin returns null, the message will be dropped.
+  if ((await hooks.aCallAll('handleMessage', {client, message})).some((m) => m === null)) {
+    return;
+  }
+
+  // Drop the message if the client disconnected during the above processing.
+  if (sessioninfos[client.id] !== thisSession) {
+    messageLogger.warn('Dropping message from a connection that has gone away.')
+    return;
+  }
 
   // Check what type of message we get and delegate to the other methods
   if (message.type === "CLIENT_READY") {