diff --git a/CHANGELOG.md b/CHANGELOG.md index 0179005a7..ad489422d 100644 --- a/CHANGELOG.md +++ b/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): diff --git a/doc/api/hooks_server-side.md b/doc/api/hooks_server-side.md index 54192ef3c..81e505108 100644 --- a/doc/api/hooks_server-side.md +++ b/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'; }; ``` diff --git a/src/node/handler/PadMessageHandler.js b/src/node/handler/PadMessageHandler.js index 34db0cbc6..a358807e9 100644 --- a/src/node/handler/PadMessageHandler.js +++ b/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(); diff --git a/src/tests/backend/common.js b/src/tests/backend/common.js index 1787ace7d..000354232 100644 --- a/src/tests/backend/common.js +++ b/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'); diff --git a/src/tests/backend/specs/messages.js b/src/tests/backend/specs/messages.js index 2d5546d6c..e36e8d98b 100644 --- a/src/tests/backend/specs/messages.js +++ b/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(rev), - sendUserChanges('Z:6>0-5+5$hello'), + 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(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'); + }); }); });