diff --git a/services/real-time/app/js/WebsocketLoadBalancer.js b/services/real-time/app/js/WebsocketLoadBalancer.js index e6a8b99879..d2cccab46d 100644 --- a/services/real-time/app/js/WebsocketLoadBalancer.js +++ b/services/real-time/app/js/WebsocketLoadBalancer.js @@ -25,6 +25,16 @@ module.exports = WebsocketLoadBalancer = { rclientPubList: RedisClientManager.createClientList(Settings.redis.pubsub), rclientSubList: RedisClientManager.createClientList(Settings.redis.pubsub), + shouldDisconnectClient(client, message) { + const userId = client.ol_context.user_id + if (message?.message === 'userRemovedFromProject') { + if (message?.payload?.includes(userId)) { + return true + } + } + return false + }, + emitToRoom(roomId, message, ...payload) { if (!roomId) { logger.warn( @@ -154,7 +164,19 @@ module.exports = WebsocketLoadBalancer = { for (const client of clientList) { if (!seen.has(client.id)) { seen.set(client.id, true) - client.emit(message.message, ...message.payload) + if (WebsocketLoadBalancer.shouldDisconnectClient(client, message)) { + logger.debug( + { + message, + userId: client?.ol_context?.user_id, + projectId: client?.ol_context?.project_id, + }, + 'disconnecting client' + ) + client.disconnect() + } else { + client.emit(message.message, ...message.payload) + } } } } else if (message.health_check) { diff --git a/services/real-time/test/acceptance/js/ReceiveEditorEventTests.js b/services/real-time/test/acceptance/js/ReceiveEditorEventTests.js new file mode 100644 index 0000000000..d12f45bcaf --- /dev/null +++ b/services/real-time/test/acceptance/js/ReceiveEditorEventTests.js @@ -0,0 +1,248 @@ +/* eslint-disable + no-unused-vars, +*/ +// TODO: This file was created by bulk-decaffeinate. +// Fix any style issues and re-enable lint. +/* + * decaffeinate suggestions: + * DS102: Remove unnecessary code created because of implicit returns + * DS207: Consider shorter variations of null checks + * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md + */ +const { expect } = require('chai') + +const RealTimeClient = require('./helpers/RealTimeClient') +const FixturesManager = require('./helpers/FixturesManager') + +const async = require('async') + +const settings = require('@overleaf/settings') +const redis = require('@overleaf/redis-wrapper') +const rclient = redis.createClient(settings.redis.pubsub) + +describe('receiveEditorEvent', function () { + beforeEach(function (done) { + this.lines = ['test', 'doc', 'lines'] + this.version = 42 + this.ops = ['mock', 'doc', 'ops'] + + /** + * We will set up a project, a doc, and three users: the owner, user 'a' and user 'b' + */ + this.project_id = null + this.doc_id = null + + this.owner_user_id = null + this.owner_client = null + + this.user_a_id = null + this.user_a_client = null + + this.user_b_id = null + this.user_b_client = null + + async.series( + [ + /** + * Create the project, doc, and owner + */ + cb => { + return FixturesManager.setUpProject( + { + privilegeLevel: 'owner', + project: { name: 'Test Project' }, + }, + (error, { user_id: userId, project_id: projectId }) => { + if (error) return done(error) + this.owner_user_id = userId + this.project_id = projectId + return cb() + } + ) + }, + + cb => { + return FixturesManager.setUpDoc( + this.project_id, + { lines: this.lines, version: this.version, ops: this.ops }, + (e, { doc_id: docId }) => { + this.doc_id = docId + return cb(e) + } + ) + }, + + /** + * Connect owner to project/doc + */ + cb => { + this.owner_client = RealTimeClient.connect() + return this.owner_client.on('connectionAccepted', cb) + }, + + cb => { + return this.owner_client.emit( + 'joinProject', + { + project_id: this.project_id, + }, + cb + ) + }, + + cb => { + return this.owner_client.emit('joinDoc', this.doc_id, cb) + }, + + /** + * add user_a to project + */ + cb => { + return FixturesManager.setUpProject( + { + privilegeLevel: 'readAndWrite', + project_id: this.project_id, + }, + (error, { user_id: userIdSecond }) => { + if (error) return done(error) + this.user_a_id = userIdSecond + return cb() + } + ) + }, + + /** + * Connect user_a to project/doc + */ + cb => { + this.user_a_client = RealTimeClient.connect() + return this.user_a_client.on('connectionAccepted', cb) + }, + + cb => { + return this.user_a_client.emit( + 'joinProject', + { + project_id: this.project_id, + }, + cb + ) + }, + cb => { + return this.user_a_client.emit('joinDoc', this.doc_id, cb) + }, + + /** + * Set up user_b + */ + cb => { + return FixturesManager.setUpProject( + { + privilegeLevel: 'readAndWrite', + project_id: this.project_id, + }, + (error, { user_id: userIdThird }) => { + if (error) return done(error) + this.user_b_id = userIdThird + return cb() + } + ) + }, + + /** + * Connect user_b to project/doc + */ + cb => { + this.user_b_client = RealTimeClient.connect() + return this.user_b_client.on('connectionAccepted', cb) + }, + + cb => { + return this.user_b_client.emit( + 'joinProject', + { + project_id: this.project_id, + }, + cb + ) + }, + cb => { + return this.user_b_client.emit('joinDoc', this.doc_id, cb) + }, + + /** + * Listen for updates + */ + cb => { + const eventName = 'userRemovedFromProject' + this.owner_updates = [] + this.owner_client.on(eventName, update => + this.owner_updates.push({ [eventName]: update }) + ) + this.user_a_updates = [] + this.user_a_client.on(eventName, update => + this.user_a_updates.push({ [eventName]: update }) + ) + this.user_b_updates = [] + this.user_b_client.on(eventName, update => + this.user_b_updates.push({ [eventName]: update }) + ) + return cb() + }, + ], + done + ) + }) + + afterEach(function () { + if (this.owner_client) { + this.owner_client.disconnect() + } + if (this.user_a_client) { + this.user_a_client.disconnect() + } + if (this.user_b_client) { + this.user_b_client.disconnect() + } + }) + + describe('event: userRemovedFromProject', function () { + let removedUserId + beforeEach(function (done) { + /** + * We remove user_a from the project + */ + removedUserId = `${this.user_a_id}` + rclient.publish( + 'editor-events', + JSON.stringify({ + room_id: this.project_id, + message: 'userRemovedFromProject', + payload: [removedUserId], + }) + ) + setTimeout(done, 200) + }) + + it('should disconnect the removed user', function () { + expect(this.user_a_client.socket.connected).to.equal(false) + }) + + it('should not disconnect the other users', function () { + expect(this.owner_client.socket.connected).to.equal(true) + expect(this.user_b_client.socket.connected).to.equal(true) + }) + + it('should send the event to the remaining connected clients', function () { + expect(this.owner_updates).to.deep.equal([ + { userRemovedFromProject: removedUserId }, + ]) + + expect(this.user_a_updates.length).to.equal(0) + + expect(this.user_b_updates).to.deep.equal([ + { userRemovedFromProject: removedUserId }, + ]) + }) + }) +}) diff --git a/services/real-time/test/unit/js/WebsocketLoadBalancerTests.js b/services/real-time/test/unit/js/WebsocketLoadBalancerTests.js index ec341a43bd..9299b0eb2d 100644 --- a/services/real-time/test/unit/js/WebsocketLoadBalancerTests.js +++ b/services/real-time/test/unit/js/WebsocketLoadBalancerTests.js @@ -11,6 +11,7 @@ */ const SandboxedModule = require('sandboxed-module') const sinon = require('sinon') +const expect = require('chai').expect const modulePath = require('path').join( __dirname, '../../../app/js/WebsocketLoadBalancer' @@ -53,6 +54,42 @@ describe('WebsocketLoadBalancer', function () { return (this.payload = ['argument one', 42]) }) + describe('shouldDisconnectClient', function () { + const client = { + ol_context: { user_id: 'abcd' }, + } + + it('should return false for general messages', function () { + const message = { + message: 'someNiceMessage', + payload: [{ data: 'whatever' }], + } + expect( + this.WebsocketLoadBalancer.shouldDisconnectClient(client, message) + ).to.equal(false) + }) + + it('should return false for userRemovedFromProject, when the user_id does not match', function () { + const message = { + message: 'userRemovedFromProject', + payload: ['xyz'], + } + expect( + this.WebsocketLoadBalancer.shouldDisconnectClient(client, message) + ).to.equal(false) + }) + + it('should return true for userRemovedFromProject, if the user_id matches', function () { + const message = { + message: 'userRemovedFromProject', + payload: [`${client.ol_context.user_id}`], + } + expect( + this.WebsocketLoadBalancer.shouldDisconnectClient(client, message) + ).to.equal(true) + }) + }) + describe('emitToRoom', function () { beforeEach(function () { return this.WebsocketLoadBalancer.emitToRoom( @@ -279,7 +316,7 @@ describe('WebsocketLoadBalancer', function () { }) }) // restricted client, should not be called - return describe('when emitting to all', function () { + describe('when emitting to all', function () { beforeEach(function () { this.io.sockets = { emit: (this.emit = sinon.stub()) } const data = JSON.stringify({ @@ -300,5 +337,66 @@ describe('WebsocketLoadBalancer', function () { .should.equal(true) }) }) + + describe('when it should disconnect one of the clients', function () { + const targetUserId = 'bbb' + const message = 'userRemovedFromProject' + const payload = [`${targetUserId}`] + const clients = [ + { + id: 'client-id-1', + emit: sinon.stub(), + ol_context: { user_id: 'aaa' }, + disconnect: sinon.stub(), + }, + { + id: 'client-id-2', + emit: sinon.stub(), + ol_context: { user_id: `${targetUserId}` }, + disconnect: sinon.stub(), + }, + { + id: 'client-id-3', + emit: sinon.stub(), + ol_context: { user_id: 'ccc' }, + disconnect: sinon.stub(), + }, + ] + beforeEach(function () { + this.io.sockets = { + clients: sinon.stub().returns(clients), + } + const data = JSON.stringify({ + room_id: this.room_id, + message, + payload, + }) + return this.WebsocketLoadBalancer._processEditorEvent( + this.io, + 'editor-events', + data + ) + }) + + it('should disconnect the matching client, while sending message to other clients', function () { + this.io.sockets.clients.calledWith(this.room_id).should.equal(true) + + const [client1, client2, client3] = clients + + // disconnecting one client + client1.disconnect.called.should.equal(false) + client2.disconnect.called.should.equal(true) + client3.disconnect.called.should.equal(false) + + // emitting to remaining clients + client1.emit + .calledWith(message, ...Array.from(payload)) + .should.equal(true) + client2.emit.called.should.equal(false) // disconnected client should not be called + client3.emit + .calledWith(message, ...Array.from(payload)) + .should.equal(true) + }) + }) }) })