PadMessageHandler: Allow handleMessageSecurity to grant one-time write access

Author: rhansen

CHANGELOG.md

@@ -35,6 +35,8 @@
 * The `handleMessageSecurity` and `handleMessage` server-side hooks have a new
   `sessionInfo` context property that includes the user's author ID, the pad ID,
   and whether the user only has read-only access.
+* The `handleMessageSecurity` server-side hook can now be used to grant write
+  access for the current message only.
 
 ### Compatibility changes
 
@@ -43,6 +45,8 @@
 * The `client` context property for the `handleMessageSecurity` and
   `handleMessage` server-side hooks is deprecated; use the `socket` context
   property instead.
+* Returning `true` from a `handleMessageSecurity` hook function is deprecated;
+  return `'permitOnce'` instead.
 * Changes to the `src/static/js/Changeset.js` library:
   * The following attribute processing functions are deprecated (use the new
     attribute APIs instead):

doc/api/hooks_server-side.md

@@ -616,12 +616,15 @@ temporary write access to a pad.
 Supported return values:
 
 * `undefined`: No change in access status.
-* `true`: Override the user's read-only access for all `COLLABROOM` messages
-  from the same socket.io connection (including the current message, if
-  applicable) until the client's next `CLIENT_READY` message. Has no effect if
-  the user already has write access to the pad. Read-only access is reset
-  **after** each `CLIENT_READY` message, so returning `true` has no effect for
-  `CLIENT_READY` messages.
+* `'permitOnce'`: Override the user's read-only access for the current
+  `COLLABROOM` message only. Has no effect if the current message is not a
+  `COLLABROOM` message, or if the user already has write access to the pad.
+* `true`: (**Deprecated**; return `'permitOnce'` instead.) Override the user's
+  read-only access for all `COLLABROOM` messages from the same socket.io
+  connection (including the current message, if applicable) until the client's
+  next `CLIENT_READY` message. Has no effect if the user already has write
+  access to the pad. Read-only access is reset **after** each `CLIENT_READY`
+  message, so returning `true` has no effect for `CLIENT_READY` messages.
 
 Context properties:
 
@@ -639,9 +642,9 @@ Example:
 
 ```javascript
 exports.handleMessageSecurity = async (hookName, context) => {
-  const {message, sessionInfo: {readOnly}, socket} = context;
+  const {message, sessionInfo: {readOnly}} = context;
   if (!readOnly || message.type !== 'COLLABROOM') return;
-  if (shouldGrantWriteAccess(message, socket)) return true;
+  if (await messageIsBenign(message)) return 'permitOnce';
 };
 ```
 

src/node/handler/PadMessageHandler.js

@@ -276,6 +276,7 @@ exports.handleMessage = async (socket, message) => {
   thisSession.author = authorID;
 
   // Allow plugins to bypass the readonly message blocker
+  let readOnly = thisSession.readonly;
   const context = {
     message,
     sessionInfo: {
@@ -291,8 +292,21 @@ exports.handleMessage = async (socket, message) => {
       return this.socket;
     },
   };
-  if ((await hooks.aCallAll('handleMessageSecurity', context)).some((w) => w === true)) {
-    thisSession.readonly = false;
+  for (const res of await hooks.aCallAll('handleMessageSecurity', context)) {
+    switch (res) {
+      case true:
+        padutils.warnDeprecated(
+            'returning `true` from a `handleMessageSecurity` hook function is deprecated; ' +
+            'return "permitOnce" instead');
+        thisSession.readonly = false;
+        // Fall through:
+      case 'permitOnce':
+        readOnly = false;
+        break;
+      default:
+        messageLogger.warn(
+            'Ignoring unsupported return value from handleMessageSecurity hook function:', res);
+    }
   }
 
   // Call handleMessage hook. If a plugin returns null, the message will be dropped.
@@ -312,7 +326,7 @@ exports.handleMessage = async (socket, message) => {
   } else if (message.type === 'CHANGESET_REQ') {
     await handleChangesetRequest(socket, message);
   } else if (message.type === 'COLLABROOM') {
-    if (thisSession.readonly) {
+    if (readOnly) {
       messageLogger.warn('Dropped message, COLLABROOM for readonly pad');
     } else if (message.data.type === 'USER_CHANGES') {
       stats.counter('pendingEdits').inc();

src/tests/backend/common.js

@@ -172,14 +172,14 @@ exports.connect = async (res = null) => {
  * @param {string} padId - Which pad to join.
  * @returns The CLIENT_VARS message from the server.
  */
-exports.handshake = async (socket, padId) => {
+exports.handshake = async (socket, padId, token = 't.12345') => {
   logger.debug('sending CLIENT_READY...');
   socket.send({
     component: 'pad',
     type: 'CLIENT_READY',
     padId,
     sessionID: null,
-    token: 't.12345',
+    token,
   });
   logger.debug('waiting for CLIENT_VARS response...');
   const msg = await exports.waitForSocketEvent(socket, 'message');

src/tests/backend/specs/messages.js

@@ -1,103 +1,141 @@
 'use strict';
 
-const AttributePool = require('../../../static/js/AttributePool');
 const assert = require('assert').strict;
 const common = require('../common');
 const padManager = require('../../../node/db/PadManager');
+const plugins = require('../../../static/js/pluginfw/plugin_defs');
+const readOnlyManager = require('../../../node/db/ReadOnlyManager');
 
 describe(__filename, function () {
   let agent;
   let pad;
   let padId;
+  let roPadId;
   let rev;
   let socket;
+  let roSocket;
+  const backups = {};
 
   before(async function () {
     agent = await common.init();
   });
 
   beforeEach(async function () {
+    backups.hooks = {handleMessageSecurity: plugins.hooks.handleMessageSecurity};
+    plugins.hooks.handleMessageSecurity = [];
     padId = common.randomString();
     assert(!await padManager.doesPadExist(padId));
-    pad = await padManager.getPad(padId, '');
+    pad = await padManager.getPad(padId, 'dummy text');
+    await pad.setText('\n'); // Make sure the pad is created.
     assert.equal(pad.text(), '\n');
-    const res = await agent.get(`/p/${padId}`).expect(200);
+    let res = await agent.get(`/p/${padId}`).expect(200);
     socket = await common.connect(res);
     const {type, data: clientVars} = await common.handshake(socket, padId);
     assert.equal(type, 'CLIENT_VARS');
     rev = clientVars.collab_client_vars.rev;
+
+    roPadId = await readOnlyManager.getReadOnlyId(padId);
+    res = await agent.get(`/p/${roPadId}`).expect(200);
+    roSocket = await common.connect(res);
+    await common.handshake(roSocket, roPadId, `t.${common.randomString(8)}`);
   });
 
   afterEach(async function () {
+    Object.assign(plugins.hooks, backups.hooks);
     if (socket != null) socket.close();
     socket = null;
+    if (roSocket != null) roSocket.close();
+    roSocket = null;
     if (pad != null) await pad.remove();
     pad = null;
   });
 
   describe('USER_CHANGES', function () {
     const sendUserChanges =
-        async (changeset) => await common.sendUserChanges(socket, {baseRev: rev, changeset});
-    const assertAccepted = async (wantRev) => {
+        async (socket, cs) => await common.sendUserChanges(socket, {baseRev: rev, changeset: cs});
+    const assertAccepted = async (socket, wantRev) => {
       await common.waitForAcceptCommit(socket, wantRev);
       rev = wantRev;
     };
-    const assertRejected = async () => {
+    const assertRejected = async (socket) => {
       const msg = await common.waitForSocketEvent(socket, 'message');
       assert.deepEqual(msg, {disconnect: 'badChangeset'});
     };
 
     it('changes are applied', async function () {
       await Promise.all([
-        assertAccepted(rev + 1),
-        sendUserChanges('Z:1>5+5$hello'),
+        assertAccepted(socket, rev + 1),
+        sendUserChanges(socket, 'Z:1>5+5$hello'),
       ]);
       assert.equal(pad.text(), 'hello\n');
     });
 
     it('bad changeset is rejected', async function () {
       await Promise.all([
-        assertRejected(),
-        sendUserChanges('this is not a valid changeset'),
+        assertRejected(socket),
+        sendUserChanges(socket, 'this is not a valid changeset'),
       ]);
     });
 
     it('retransmission is accepted, has no effect', async function () {
       const cs = 'Z:1>5+5$hello';
       await Promise.all([
-        assertAccepted(rev + 1),
-        sendUserChanges(cs),
+        assertAccepted(socket, rev + 1),
+        sendUserChanges(socket, cs),
       ]);
       --rev;
       await Promise.all([
-        assertAccepted(rev + 1),
-        sendUserChanges(cs),
+        assertAccepted(socket, rev + 1),
+        sendUserChanges(socket, cs),
       ]);
       assert.equal(pad.text(), 'hello\n');
     });
 
     it('identity changeset is accepted, has no effect', async function () {
       await Promise.all([
-        assertAccepted(rev + 1),
-        sendUserChanges('Z:1>5+5$hello'),
+        assertAccepted(socket, rev + 1),
+        sendUserChanges(socket, 'Z:1>5+5$hello'),
       ]);
       await Promise.all([
-        assertAccepted(rev),
-        sendUserChanges('Z:6>0$'),
+        assertAccepted(socket, rev),
+        sendUserChanges(socket, 'Z:6>0$'),
       ]);
       assert.equal(pad.text(), 'hello\n');
     });
 
     it('non-identity changeset with no net change is accepted, has no effect', async function () {
       await Promise.all([
-        assertAccepted(rev + 1),
-        sendUserChanges('Z:1>5+5$hello'),
+        assertAccepted(socket, rev + 1),
+        sendUserChanges(socket, 'Z:1>5+5$hello'),
+      ]);
+      await Promise.all([
+        assertAccepted(socket, rev),
+        sendUserChanges(socket, 'Z:6>0-5+5$hello'),
       ]);
+      assert.equal(pad.text(), 'hello\n');
+    });
+
+    it('handleMessageSecurity can grant one-time write access', async function () {
+      const cs = 'Z:1>5+5$hello';
+      // First try to send a change and verify that it was dropped.
+      await sendUserChanges(roSocket, cs);
+      // sendUserChanges() waits for message ack, so if the message was accepted then head should
+      // have already incremented by the time we get here.
+      assert.equal(pad.head, rev); // Not incremented.
+
+      // Now allow the change.
+      plugins.hooks.handleMessageSecurity.push({hook_fn: () => 'permitOnce'});
       await Promise.all([
-        assertAccepted(rev),
-        sendUserChanges('Z:6>0-5+5$hello'),
+        assertAccepted(roSocket, rev + 1),
+        sendUserChanges(roSocket, cs),
       ]);
       assert.equal(pad.text(), 'hello\n');
+
+      // The next change should be dropped.
+      plugins.hooks.handleMessageSecurity = [];
+      await sendUserChanges(roSocket, 'Z:6>6=5+6$ world');
+      assert.equal(pad.head, rev); // Not incremented.
+      assert.equal(pad.text(), 'hello\n');
     });
   });
 });